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

Add/set weather variables #9326

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

rcichota
Copy link
Contributor

Working on #9325
updating the code to:

  • fix a few typos and minor organisation of the code
  • allow a few weather variable to be settable
  • add a few extra variables

…s private), deleted variable "First' (and related lines) as not needed (replaced with test for StartDate)
…and CO2, standardised co2 to be read from met or set via manager, etc.
@hut104
Copy link
Contributor

hut104 commented Sep 18, 2024

Our Weather models seem to be getting pretty complex (with several models and lots of public variables and/or setters).
What are some of the use cases here? e.g. why is latitude settable?
If someone changes latitude, I assume that daylength is recalculated, as are dates of seasons, etc. There are lots of interdependencies between weather variables. If one gets changed, do we handle the dependencies?

I also see repeated patterns of if statements all through the model suggesting a refactoring may be needed.

@rcichota
Copy link
Contributor Author

Good points @hut104 , I am aware that there are many interdependencies, and so trying to keep that in mind when doing any changes. I will set a few tests regarding the potential flow on effect of setting Latitude. I will revert if needed or advised on that.
I've got into doing this as me and @hol353 needed to set some of the weather variables (including latitude, mean T and PET) for our work with soil temperature models in Montpellier. The case to make this general is that is allows (as is needed for that work) to set controlled conditions and isolate more effectively some models for testing and sensitivity analysis. Looking at the code I noticed that the weather variables were defined/set in somewhat inconsistent manner, so I am trying to standardise things a bit (also add a few variables that may be useful, eg. PET, but as this would only be made available to models, would not change SoilWater). I have used the same structure that was in the model, I agree there are repeated patterns, it would be good if someone with more coding skills had a look at it.
Certainly will need a good revision of this before merging

@rcichota
Copy link
Contributor Author

rcichota commented Sep 19, 2024

My idea is to standardise the behaviour for setting weather variables, firstly the code checks if daily values were given (as a column in met file), if not then check if there is a single value (constant), finally providing a 'default' value (which is 'null' for required data (Rain, MaxT, etc.), a constant (for Wind or CO2), or a value is calculated using a function (DiffuseFraction, VP, etc.).
Obviously, this does not apply to values expected to be constants (like Latitude). Moreover, users could set the variables during 'OnPreparingNewWeatherData', all derived values (dependencies) should be calculated after this (or on their getters).

@hol353
Copy link
Contributor

hol353 commented Sep 24, 2024

You’re on the right track. I think what you’re coming up against is the poor design by the weather components brought about by a design that has evolved over time.

What we need is a single place (model) that stores and checks weather data and makes it available to other models that need it. It would be quite a simple model (called 'weather' like it is now). We could then have another model (as a child model maybe) that reads weather data in APSIM format, perhaps called WeatherReader (and maybe another in WTH format - called WthWeatherReader) and stores the values in 'weather'. No need for ClimateControl/ControlledEnvironement as that would be a weather component without a reader. No need for SimpleWeather either I suspect. I think Hamish wrote SimpleWeather so might need to check with him. WeatherSampler would just sample from 'Weather'. The current GUI weather graphs would also work with 'Weather'.

This might be a step too far for this PR.

@rcichota
Copy link
Contributor Author

I like the idea of refactoring and making sure that things don't get duplicated. I will be 'finishing' this soon by making sure WeatherSampler has the same variables / same behaviour. I will then open a new issue for refactoring, but will need some help from someone with more coding skills to separate the code properly...

@rcichota
Copy link
Contributor Author

rcichota commented Oct 6, 2024

I need a bit of help here, @hol353
Jenkins is saying that there's an error when running the unit test for WeatherSampler (it seems the variable 'SplitDate' has not been set, somehow - although it should have a default). I had a good look at the code and couldn't find any clue. I do not know how to run the UnitTests so cannot debug locally...

@ric394
Copy link
Contributor

ric394 commented Oct 8, 2024

@rcichota for vs code:

run_unit_tests

for visual studio:

run_unit_tests_vs

@hol353
Copy link
Contributor

hol353 commented Oct 8, 2024

@rcichota - Fixed.

@rcichota
Copy link
Contributor Author

rcichota commented Oct 8, 2024

Thanks @ric394 , I can now run the units test locally.
And thanks @hol353 for fixing the issue with the WeatherSampler UnitTest.
I have another issue, I am not able to replicate the results from validation tests (AgPasture.apsimx). There seem to be a significant issue with one of the sites in the validation (Ranui, see RngAnnualHarvestedWt), but when I run locally, the diffs I get are very small (almost negligible, e.g. my RMSE for that graph is 2689.70). Not sure how this is possible, will continue looking into...

@rcichota
Copy link
Contributor Author

rcichota commented Oct 8, 2024

retest this please Jenkins

@rcichota
Copy link
Contributor Author

rcichota commented Oct 9, 2024

@hol353
I haven't been able to replicate the results from Jenkins, and neither could Xiumei. This is evident on the AgPasture simulation. Could be that the test system is doing something odd?
I just don't know what action I could try to solve the diffs if I cannot replicate the runs...

@hol353
Copy link
Contributor

hol353 commented Oct 9, 2024

@rcichota: I can reproduce the results on jenkins if I use a linux computer (which is what Jenkins is using). I've spent a couple of hours trying to work out what isn't working correctly on linux and I can't. I've tried lots of things. The problem is that this PR has a lot of changes (rearranged code and some more semi-major changes) in a lot of commits and it has never run green on Jenkins.

I can't spend more time on this. You might have to go back to the beginning and reintroduce the changes slowly and carefully.

As I've said several comments above, my interest is separating out the weather file reading code into a separate class and refactor out the helper code (e.g. calculating things like tav/amp/daylength/airpressure etc) into a separate weather utilities class. Perhaps we abandon this PR and start again.

@rcichota
Copy link
Contributor Author

Thanks @hol353
Much appreciated the time you spend on this. Still odd that the results in Linux are so different than in Windows (I get 3.6t of biomass, while Jenkins gest more than 10t, for the Ranui site). The code has run green many times (I am sure), only when I changed how AirPressure is output, it went red (and mostly only for AgPasture, as the value now respond to Altitude and that simulation actually has values for it). Funny it is showing all commits with crosses now...
Regardless, I quite agree with you on starting anew (there were lots of changes indeed). But then it would be better to start 'properly', with the code set up as you proposed a few comments earlier. I am happy to put a bit of time and get this over the line (mostly would be checking/copying what was done here), but I need help to get started, get the basic models/files/classes in place then I can put in the actual code. Do you, or any of the guys there, could help with that?

@peter-devoil
Copy link
Contributor

My apologies for introducing simpleweather. The context was a ML application where the simulation is continually restarted, but not re-initialised (ie after link resolution), sometimes using a different weather file. I couldnt see any sense in a linked list that caches yesterdays data, some other application must be using it, so I left it in place. It is also a hit if we want to have one enormous weather file and randomly sample from it.

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.

5 participants