Skip to content
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

Apply overrides in Land and surfaces #82

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft

Conversation

liuly12
Copy link
Collaborator

@liuly12 liuly12 commented Mar 5, 2024

Apply overrides in Land and Surface and its superclasses.

@liuly12 liuly12 linked an issue Mar 5, 2024 that may be closed by this pull request
@liuly12 liuly12 self-assigned this Mar 5, 2024
Copy link
Collaborator

@barneydobson barneydobson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly just some stylistic suggestions - otherwise happy

tests/test_land.py Outdated Show resolved Hide resolved
@@ -903,7 +903,7 @@ def test_soil_pool(self):
d1["org-phosphorus"] = disso["P"]
d1["org-nitrogen"] = disso["N"]

self.assertDictAlmostEqual(d1, r1, 15)
self.assertDictAlmostEqual(d1, r1, 14)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it - why did the normal tests change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the odd one and I'm not very sure what the reason is. After applying the overrides, if I leave it as 15 float accuracy it will report error, but 14 is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh that number is float accuracy - no problem from me then

Comment on lines +391 to +392
self.surface = overrides.pop("surface",
self.surface)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that someone would be doing? I wonder if it would interact with data reading?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on our choices of promoting the 'standard' way I guess, but happy to close this portal if we only want the user to change the surface name via yaml.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think remove it probably - otherwise we will need to figure out whether a bunch of stuff (i.e., things around the data_input_dict reading/loading/etc) would need to change. We can make a separate (low priority) issue for this if you think.

wsimod/nodes/land.py Outdated Show resolved Hide resolved
wsimod/nodes/land.py Outdated Show resolved Hide resolved
be overridden (keys) and new values (values). Defaults to {}.
"""

self.depth /= self.total_porosity # restore the physical depth (root)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe depth should be a @property? Possibly is too linked with other things, not sure

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to try and test. I'll flag this in an issue and try to revise it when I get a bit more time. #95

for param in overwrite_params:
setattr(self, param, overrides.pop(param, getattr(self, param)))

self.subsurface_coefficient = 1 - self.percolation_coefficient
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also @property ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#95

self.autumn_sow = True
else:
self.autumn_sow = False
self.harvest_sow_calendar, self.ground_cover_stages, self.crop_cover_stages, self.autumn_sow = self.infer_sow_harvest_calendar()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see some functions!

(float): readily available water
"""
# Calculate parameters based on capacity/wp
total_available_water = self.field_capacity_m - self.wilting_point_m
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should TAW/RAW just be a @property maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#95

}
fraction_dry_n_to_fast = 1 - self.fraction_dry_n_to_dissolved_inorganic

return fraction_manure_to_fast, fraction_residue_to_humus, fraction_dry_n_to_fast
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to just store these in self inside the function - same as some functions earlier I think. Not sure what the motivation of passing them about is

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree - see commit 9641dbf

liuly12 and others added 4 commits April 10, 2024 08:53
Co-authored-by: barneydobson <barnaby.dobson1@gmail.com>
Co-authored-by: barneydobson <barnaby.dobson1@gmail.com>
Co-authored-by: barneydobson <barnaby.dobson1@gmail.com>
@liuly12 liuly12 linked an issue Apr 10, 2024 that may be closed by this pull request
@liuly12 liuly12 requested a review from dalonsoa April 10, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Land overrides using @property Land overrides
2 participants