-
Notifications
You must be signed in to change notification settings - Fork 30
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
ModelComponent Class #769
ModelComponent Class #769
Conversation
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.
Looking good Tjalling. I think this is nice for a first try and we'll see about polishing some things when we start integrating it with the rest and moving over tests.
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.
Hi @Tjalling-dejong , very nice first implementation of the Grid ModelComponent!
I think it's almost ready, I just added a few answers to Sam and asked to rename the setup methods but for the rest almost ready to merge I think :)
Just one thing, but before you merge, I think we don't need the v1 folder and maybe rename grid.py script to grid_component.py
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.
Some quick suggestions:
- we need to initialize a component with the
model
rather than justroot
to be able to acces the model region, data catalog, etc.. Using a model property that calls the weak_ref to model we can avoid circular dependencies, see also discussion New `ModelComponent` class #636 - it would be good to keep the 'just in time reading' we have currently. This is achieved by creating a
data
property which calls_initialize_data
if_data is None
. In most other methods we can than simply usedata
rather than_data
, see also current implementation for grid, maps, geoms, etc. - Both the
model
anddata
properties should land in the genericModelComponent
class
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.
Almost there @Tjalling-dejong ! Just a few minor suggestions before this is finalized and can be merged :)
After this PR it would be good to harmonise and finalise the link of GridComponent
to ModelRegionComponent
and to update and link GridComponent
to Model
so that we can create gridded models again.
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 broadly agree with Hélène's comments so once you resolve those I'm happy for it to be merged. Nearly there!
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
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.
Good busy! Looks like a solid PR. I assume we can get those tests up and running as soon as the Model class is also ready.
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.
Great work @Tjalling-dejong ! Looking good now :)
I still have a couple of very little comments but at the level of for example an argument that you forgot to remove from the docstrings. So would be great if you could still update before merging. But I approve already as they are quite minor.
Great job!
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Co-authored-by: hboisgon <45457510+hboisgon@users.noreply.github.com>
Issue addressed
Fixes #671
Explanation
Creating a model component subclass for grid model first. The idea is to make a general ModelComponent class that will replace the different model classess with subclasses of ModelComponent.
Checklist
main
Additional Notes (optional)
Add any additional notes or information that may be helpful.