-
Notifications
You must be signed in to change notification settings - Fork 38
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
Reading compatibility #425
Conversation
@@ -56,6 +57,81 @@ def ang_lightcone(ic, lc): | |||
) | |||
|
|||
|
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.
Ideally I would also test for gaps and gluts in the data structures themselves, but the different way coevals and lightcones are structured make this difficult.
Lightcones are agnostic to the types of the lightcone fields, so missing fields are ignored and extra ones are simply loaded in.
Coevals on the other hand rely on the output structs, so both missing and extra output fields will raise errors, and changing this behaviour requires adding safe
flags to all the lower level reading as well.
@@ -936,10 +936,23 @@ def save(self, fname=None, direc=".", clobber: bool = False): | |||
return self._write(direc=direc, fname=fname, clobber=clobber) | |||
|
|||
@classmethod |
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.
I had a question here. Where does the Coeval object read in the parameter structures? The Coeval __init__
takes them directly from the BrightnessTemp
struct, but these don't seem to be saved with the structures looking at the _write_particulars
function.
|
||
# check that missing fields are set to default | ||
assert lc2.user_params.BOX_LEN == UserParams._defaults_["BOX_LEN"] | ||
assert lc2.global_params["OPTIMIZE_MIN_MASS"] == global_params.OPTIMIZE_MIN_MASS |
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.
This assert would be more useful if I changed the global_params in the lc/coeval fixtures
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #425 +/- ##
==========================================
+ Coverage 82.19% 82.23% +0.04%
==========================================
Files 13 13
Lines 3140 3153 +13
Branches 854 861 +7
==========================================
+ Hits 2581 2593 +12
Misses 400 400
- Partials 159 160 +1 ☔ View full report in Codecov by Sentry. |
There is currently a behaviour in
global_params.use()
which, unlike other parameter structures, raises an error when a keyword not in the parameter set is passed. This made it impossible to read in boxes from different versions that had differing parameters. I've made it behave like the other parameter sets now, which simply send out a warning.However, to keep things safer, I've added a flag to the high-level output reading functions which ensure that all the parameters are there, if the flag is on and the parameters are mismatched it raises an error. if the flag is off it emits a warning detailing which parameters are missing / extra.
Tests have also been written to ensure the desired behaviour.