-
Notifications
You must be signed in to change notification settings - Fork 14
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
Pydantic ColocationSetup #1120
Pydantic ColocationSetup #1120
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main-dev #1120 +/- ##
============================================
+ Coverage 78.82% 79.24% +0.41%
============================================
Files 128 129 +1
Lines 20209 20213 +4
============================================
+ Hits 15930 16018 +88
+ Misses 4279 4195 -84
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
640cbdb
to
2773379
Compare
4691c71
to
4242cfc
Compare
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.
Looks good. Minimal rather internal api-changes, i.e. col.model_id
-> col.colocation_setup.model_id
and eval_setup.colocation_opts["start"]
to eval_setup.colocation_opts.start
. Please document these.
Some no longer needed comments are still in the code and should be removed.
Thanks. I just checked and (1) still works, (2) only occurs in private methods. Should we still document this given this class is based on pydantic and pydantic doesn't allow this functionality? |
If (2) only occurs in private methods, we don't need to document or implement the no-longer existing dict-like behaviour. |
Change Summary
This PR does quite a bit. Changes include:
ColocationSetup
in terms of Pydantic. Move this class into it's own module:colocation_setup.py
Colocator
is no longer a child class ofColocationSetup
. To get an instance of this class you must provide and instance ofColocationSetup
or a dictionary. The option to pass a dictionary comes with a deprecation warning that it will be removed in future versions. Many tests have been changed to reflect this preferred method of instantiating aColocator
, where first one declares an instance ofColocationSetup
and then passes this toColocator
. Dependency injection was preferred in this case to class inheritance in favor or simplifying the code between classes which contain data and classes which manipulate data.ColocationSetup
have been defined more rigorously based on the expected behavior in the code. For example,obs_vars
is type-hinted to be a tuple (of strings) to indicate to programmers that it is immutable.start
andstop
if provided as strings will be coerced into pandas Timestamps. Many of these changes have been changed in config files.EvalSetup
now has it'scolocation_opts
created using same method that other pydantic-based attributes ge filled. This is an area for future improvement once the config file structure is decided.get_colocator
has been hacked in a way to get this work, and probably could be made better. That said. this function goes against how I think a simpler version of pyaeroval would work. So some design should take place so that it isn't needed.Colocator
andColocationSetup
have been removed, because this behavior is no longer supported nor encouraged.It is worth noting that
ColocationSetup
ideally would be immutable. I have left comments in the PydanticConfigDict
of this class to keep it in mind that this should be a future goal. Once you define a collocation setup at the beginning of your experiment, that should be it. And additional data that get discovered along the way should be placed somewhere else. Also I would likevalidate_assignment=True
to be enabled, but it is currently commented out as it breaks the CAMS2_83 CLI, and so more work would be needed to make this change and probably some changes on the website.Related issue number
Part of #1085. Works towards defining the interface for a
ColocatedData
object which we will need data validation on (#857).Checklist