537 config class #1072
537 config class #1072
Conversation
…d fadingfunction config sections.
Added dataclasses for: saturation, fov, linkingfilter, output, lightcurve, activity and expert config sections.
Fixed some errors in the dataclasses and also checked and created unit tests for all dataclasses
Fixed errors in dataclasses. Went through each function that called configs and changed them to call sconfigs. Changed all called variables from the configs dictionary to instead call all sconfigs dataclass attributes.
Fixed simulationConfigs and also changed the PrintConfigsToLog function to work with dataclasses
added short descriptions to each unit test in test sorchaConfigs. Also changed function _read_configs_from_object in sorchaConfigs to loop through all the config sections when populating dataclasses.
linting file
|
@astronomerritt I’m not sure these function calls in the if statement are actually doing anything given the floats and other variables are being initialized with default values at the start of each class . Can you double check? |
|
@Little-Ryugu it looks like you’ve uploaded new golden csv file. Were the tests failing? If so then something changed in sorcha from the new config file class. We might need a new PR where you copy in the changes into a fresh branch because we can’t have the end to end units tests changing otherwise the repo is not protected against changes. Your updates should not have changed the goldens. The output from the end to end tests. |
| args.configfile, | ||
| args.surveyname, | ||
| ) | ||
| configs["seed"] = 24601 | ||
| #configs["seed"] = 24601 |
There was a problem hiding this comment.
if this is not being used get rid of the line don’t comment it out
tests/data/PPConfig_goldens_test.ini
Outdated
There was a problem hiding this comment.
these config files should not be changing. What happened?
There was a problem hiding this comment.
I changed the config files because I had checks in the fovConfig making footprint_edge_threshold mandatory and also for when ephemerides_type is ar in simulationConfigs. I can remove those checks and then they should be able to pass those unit tests without changing the golden config files.
There was a problem hiding this comment.
checking ephemerides_type is ar when the other bits about the ephemeris simulation are specified is correct - you spotted a edge case the old config file parser missed.
There was a problem hiding this comment.
footprint_edge_threshold is not mandatory. It's set to None if the old config parser if it does not exist in the config file and it being none triggers the simpler camera footprint that deals with detectors. So we'll need that change made
PS (not need to respond to me outside of hours - evenings or weekends may be the only time I can take a look)
patching config and goldens to what is in the main
|
I was able to copy over what was in the main branch back into the golden csv files and the modified config files and push that to this branch |
we had two test config files that had the parameters for the A+R ephemeris generation but is reading in from an external file instead, so I commented out the A+R stuff
|
So looking at where pytest is failing. Nicely done spotting two test config files were we specified it was running with an external ephemeris file but we had the parameters for A+R ephemeris generation uncommented out. So I've commented those out those lines, because these tests are using a pre-computed ephemeris file. I've pushed that change back to the branch. So now all the failed tests are dying with SystemExit: ERROR: No value found for required key footprint_edge_threshold in config file. Please check the file and try again. footprint_edge_threshold is not required every time the footprint filter is run. If it is specified in the config then it triggers the more complicated footprint filter. If it's not present then that's fine and the footprint filter is run that just deals with the detector and goes to the edge of the detector, not the additional bit to cut around the edge. So this should be adjusted in the config file data class. If footprint_edge_threshold is specified then you need all the parameters of rest of the more detailed footprint camera filter, but you can use the camera fooprint filter without having this config parameter included. |
removed flag not needed for footprint_edge_threshold. PPConfig_test_unchunked.ini has ephemerides_type = ar. so commented out simulation section in this config file.
When the simulationConfigs object is created in sorchaConfigs, it's being initialised with a dictionary of actual values, so the if statement calls do trigger. If you were to initialise an empty instance of simulationConfigs then yeah, it wouldn't trigger, but nobody should ever be doing that outside of (potentially) testing. |
|
This looks great though, good work. The methods on the classes need to have properly formatted docstrings, however. |
|
@astronomerritt can you modify one of the methods to show appropriate doc string format? |
Ryan already Slacked me to ask, I think he's got it :) |
post_readin_ephem_nonprimary.csv
Outdated
There was a problem hiding this comment.
This file shouldn't be in the repo - looks like someone (me) left a test statement in sorcha.py which creates it.
| class inputConfigs: | ||
| """Data class for holding INPUTS section configuration file keys and validating them.""" | ||
|
|
||
| ephemerides_type: str = "" |
There was a problem hiding this comment.
Meg has convinced me that all defaults should be None, so:
ephemerides_type: str = None
This makes it clearer that the attribute is unpopulated. Empty strings and zeros do evaluate as False in Python, but they can also cause confusion - especially when a zero (or an empty string) is a legitimate, wanted value.
|
|
||
| """ | ||
|
|
||
| if not value: |
There was a problem hiding this comment.
Likewise, once all the defaults have changed to be None, this should be:
if value is None:
to make it a more explicit check.
| """ | ||
|
|
||
| # checks to make sure value doesn't exist | ||
| if value: |
There was a problem hiding this comment.
Likewise this should be:
if value is not None:
Basically we shouldn't be relying on Python evaluating 0 and "" as False, we should explicitly use None for unpopulated attributes.
| ---------- | ||
| None | ||
| """ | ||
| # make sure all the mandatory keys have been populated. |
There was a problem hiding this comment.
You also need to check to make sure that self._ephemerides_type is in the list ["ar", "external"] using check_value_in_list(), as it has to be one of those two.
Why the underscore in front of self._ephemerides_type, by the way?
There was a problem hiding this comment.
self._ephmerides_type is read into simultionConfigs from the inputConfig class, so a check has already been made to ensure that it is in that list. Should I create the check anyway for when simulationConfigs is tested independently?
I put an underscore in front of it to indicate that it should only be used for checks in simulationConfigs because ephemerides_type is called from the inputConfigs class for the rest of the code.
There was a problem hiding this comment.
Oh, I see what you've done! Nah, you're good, that's good practice.
|
Good work, by the way. With a code change this big there's always tweaks to be made, but I can't find any more issues and the unit tests look good. |
|
And you've helped pick out 2 bugs. One you fixed with the new config class and one that an errant temp file is being created when ephemeris is read in - I'll be fixing that shortly |
|
I've deleted the temp csv file in the PR that was being created |
Changed all default attributes in dataclasses to be None and fixed unit tests in test_sorchaConfigs
|
I'm very happy with this. I've approved. @mschwamb I will leave it to you to merge in case there's anything you want to change. |
Fixes #537
Fixes #1075
make a config class.
Review Checklist for Source Code Changes