-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] compute and save the ISRF in every cell #231
base: main
Are you sure you want to change the base?
Conversation
Thanks for all your work on this! Will try and review soon 😊 |
@dnarayanan - could you rebase this on the latest |
Happy to ! (Will need to be Monday likely). Just to confirm since I’m not super with git, I want to follow a procedure like this one (that I googled)? |
Yes similar to that - but make sure you use this repository as the one you are rebasing on. So assuming you don't already have this repository set up as 'upstream':
You'll then need to do:
to force push the changes since the git history has changed. I'll keep a backup of your work here in case anything goes wrong 😄 |
thanks for the step by step :) i would have never figured out that git order of operations. i've just rebased (i believe)! |
Great! There are some real test failures, you can run the tests locally by running:
|
thanks tom! so i have to admit i'm not quite sure how to interpret the tox results. when i run the test locally, i get:
but i'm guessing that that's not the issue for why the tests are failing. when i click on the details of the tox tests reported here in github, i'm not sure what they're reporting. very happy to hunt these down, but i'm afraid i'm not even sure what i'm looking for here. |
indeed, a license does seem to exist:
|
hi tom - it seems in some tests of mine, that i find that the ISRF saved is much higher in runs where i employ more than 1 MPI core. do you have any sense as to where i might begin looking in the code to debug this? i can't tell what might be going on...if there's a normalization by the number of cores that needs to happen somewhere perhaps. it's not something as simple as the ISRF with 32 cores (for example) is 32x the ISRF energy deposited for a single core...it's like many orders of mag higher. |
@dnarayanan - sorry for not getting back to you sooner. I am currently in the process of trying to fix compatibility of Hyperion with recent Python/Numpy/Astropy versions and will take a look at this soon. |
sounds good thanks! |
@dnarayanan - now that I've unbroken the CI, could you rebase on top of the latest |
…on of frequency. note, this has a manual setting of frequencies and right now only computes, not saves
…d m.read is broken still with the new specific_energy_sum_nu array dimensions
…in now when running without ISRF calculation turned on
… naturally, random segfaults]
sorry about the delay - i just rebased and pushed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About to try this out, but spotted a typo below. Note that you can run the tests locally using e.g.:
tox -e test-nobinaries
this tests just the basic functionality of the Python package excluding tests that have to run the fortran binaries. You can then test more extensively with:
tox -e test
I'll try and look into the MPI issue shortly.
@@ -719,6 +743,7 @@ def read_run_conf(self, group): # not a class method because inherited | |||
self._read_max_reabsorptions(group) | |||
self._read_pda(group) | |||
self._read_mrw(group) | |||
self._reada_isrf(group) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here - should be read_isrf?
thanks for finding the typo! i'm trying to figure out how to push the fix but i think i need to set a rule for reconciling divergent branches after the rebase before i can push to github. any advice is helpful!
not sure which of the 3 options above are best |
Can you let me know the output of 'git remote'? |
|
Thabks! And the output of 'git log' and 'git status'? |
|
Thanks! Will take a close look this evening and suggest a fix |
the upshot is that by setting in the python code:
m.compute_isrf(True)
hyperion will save the spectrum of energy deposited in every cell. this can be accessed in the hdf5 file via:
where the units are erg/s/g (and the "g" is the dust mass in the cell).
this, however, doesn't work for AMR grids because I'm having trouble understanding the file structure, and am unable to get the code to compile. As a result, I'm labeling this PR as a work in progress, and happy to iterate on this. Two main areas I could use eyes from @astrofrog are is: