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

Implement flexible output Times #113

Open
wants to merge 26 commits into
base: development
Choose a base branch
from
Open

Conversation

AliceHarang
Copy link
Collaborator

@AliceHarang AliceHarang commented Aug 8, 2024

This is link to the task: Add more fexibility in time of output (Projects for FY24/25)

  • : Reading a vector input in the shape: t_init : t_steps : t_end
  • : Reading a vector in the shape: t1,t2,t3, ... , tn
  • : Modifying the timing for outputs
  • : Add / review sanity checks and initialisations of the XParam values
  • : Testing the two different forms
  • : Testing with dates
  • : Adding the Toutput vector to the Zone_output input shape
  • : Modifying the code to take the appropriate Toutput vector
  • : Add a automatic test
  • : Add in documentation

AliceHarang and others added 12 commits August 8, 2024 11:54
Might need to be remove to add a full vector of times for init
read (general, need zones), init (general+ zones), variables (need zones), Creation of vector time
Fix compilation error and tests still to be done too
Reading the flexible lines (10::30, 52, 60)
Just Testcase to go
@AliceHarang
Copy link
Collaborator Author

Hi Cyp,
I have finish most of this branch.

Some little points where I would like to have your point of view:

  • The user can give now vector time outputs in the form: "t_init : tsteps : t_end, value1, value2 ..., valueN". I kept the comma in the last part. I produce nc file not regular in times.
  • I use an index to know if I output this zone, located in the Model.Block.zone, should it be in loop instead?
  • I have conserved the "Outputtimesteps" entrance as an "easy / partial" input. Should I remove it for more consistency?
  • Happy to discuss date testing and how it fits in.

@AliceHarang
Copy link
Collaborator Author

Things to check:

  • merging of the Write nc files: Save2Netcdf around l.1550
  • duplicate of allocation of TSnodesout (MemManagement.cu l.194)

@CyprienBosserelle
Copy link
Owner

Results from my stress testing:

  • I agree we should keep the outputtimestep = mechanism for backward compatibility

  • I wonder if it is worth adding a compulsory initial output and endtime output? i.e. the user will specify touput= 50,3600,5000 bt that implies touput= 0,50,3600,5000,endtime

  • We need a sanity check step that the list is monotonically increasing.

  • time range is not working properly

In the code.

I would have though it is easier to convert range to vector directly in the read input. While that may not be very efficient its a pretty low cost overall and easier to maintain in the future

@CyprienBosserelle
Copy link
Owner

Correction

  • Adding start time and end time was done automatically if no range was specified. So I just removed the range capability and instead interpret the rang at readinput time.
  • Also there was a bug that did strange things because the "in zone" output time vector was not sorted. I also noticed that the new function SaveInitialisation2Netcdf() was doing something that was already in Save2Netcdf() so I removed it.
  • Using real dates is still a little bit of a challenge but I will think of something

@AliceHarang
Copy link
Collaborator Author

Hi @CyprienBosserelle , I saw that you directly read in to create the vector by putting it in "val".
Did you managed to keep the default "OutputTimeSteps" by doing this?

@CyprienBosserelle
Copy link
Owner

Not yet, thanks for the reminder. I also broke the test so I need to redo it. I was thinking of doing that in the Sanity check.
Most of my changes are just to try to setup for using real dates. but the more I think about it the more difficult it seems...

@AliceHarang
Copy link
Collaborator Author

I haven't thought it too much but if we keep the initial structure for the OutputT red but change it in string, we can see if there is a time reference and read them accordingly in the sanity check (as digit or date)?

I suppose all of this would be done in the sanity check after just reading basically the input in the readInput.

Happy to implement this if you this it can work!

@CyprienBosserelle
Copy link
Owner

CyprienBosserelle commented Aug 24, 2024

Now with Dates!

Ok I shuffled a few things around and only had time to do minimal testing. But so far it works!

how to use:

User can now use Toutput for range or single value(s) as long as they are separated by a ,. Multiple ranges can be given and a mix of single values and range.

Ranges are defined with pipe symbol |

While : is commonly used to define ranges it wouldn't work for us. that is because we reserve: for separating time with specifying a date and there would be no way to distinguish between date and range.

Toutput=5.5,0|2.2|4,3.0

Time given in Toutput is either an absolute date or a time relative to the model start time

Toutput=3600.0 means 1 hour after start time!

Now supports units

time can be given with a unit.
Toutput=5days,0|2.2s|4h,3.0min

supported units are:
second= { "seconds","second","sec","s" }; as 1sec
minute = { "minutes","minute","min","m" }; as 60sec
hour = { "hours","hour","hrs","hr","h" }; as 3600sec
day = { "days","day","d" }; as 24*3600sec
month = { "months","month","mths", "mth", "mon" }; as 3600.0 * 24.0 * 30.4375 sec
year = { "years","year","yrs", "yr", "y" }; as 3600.0 * 24.0 * 365.25

Dates are also supported

as unique dates or ranges:
Toutput=2020-01-01T00:00:00|2.2s|2020-01-01T00:00:04,2020-01-01T00:00:03

Note that range step can't be a date but instead need to be second or have a unit attached

@CyprienBosserelle
Copy link
Owner

@AliceHarang I think this is done but I'd like to have your opinion about those last changes.
I may be doing a little more clean up but I'm happy the way this works.
Cheers

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.

2 participants