kevinwolz / hisafer

An R toolbox for the Hi-sAFe biophysical agroforestry model
6 stars 4 forks source link

Update hisafer based on Hi-sAFe v4.1.0 updates #145

Closed kevinwolz closed 3 years ago

kevinwolz commented 4 years ago

FUNCTION UPDATES

UPDATE PARAMETER VALUES

REMOVE INPUT PARAMETERS

ADD INPUT PARAMETERS

REMOVE OUTPUT VARIABLES

ADD OUTPUT VARIABLES

kevinwolz commented 4 years ago

@LECOMTE-ISADEV & @cdupraz: Based on the committed updates to Hi-sAFe in v3.6, here is a list of updates that I think I need to make to hisafer and its accompanying documentation and template files. Can you help me check this to make sure I am not missing anything?

kevinwolz commented 4 years ago

@LECOMTE-ISADEV I have added some comments to your description of the 3.6 updates. Can you take a look and answer my questions?

Hisafe-3.6-KWOLZ_QUESTIONS.docx

mariegosme commented 4 years ago

@kevinwolz please do not update hisafer next week ! we have a course on hisafe and hisafer based on hisafe 4.0 (2018-11-09 version), it's not the right time to make changes.

kevinwolz commented 4 years ago

@mariegosme, don't worry I have created a new branch of hisafer (https://github.com/kevinwolz/hisafer/tree/hisafe3.6) to which I will commit the updates specific to this new Hi-sAFe version. Once the dust settles on the new Hi-sAFe version, we can then merge the branch to to master.

LECOMTE-ISADEV commented 4 years ago

Hi Kevin Sorry I don't have time this week. Next will be better Here is the responses to your questions Hisafe-3.6-KWOLZ_QUESTIONS.docx

kevinwolz commented 4 years ago

It looks like the addition of the SimulationDate header variable does not actually require any updates to hisafer. Good job past Kevin for ensuring the code was robust enough to handle the addition of header variables like this! I had thought there might be an issue in these cases:

1) The diagnostic functions, because they need to know which variables are actual outputs vs are just header variables. However, the diag functions just need to know the "last" column that is a header variable (idTree for trees, y for cells, and JulianDay otherwise. So, as long as we are not changing the last header column, the total number of header columns doesn't really make a difference.

2) In the initial reading of the files, I thought there might be an issues with joining, but it seems like not!

HOWEVER, While I'm glad there are no coding changes in hisafer required, I do think that this SimulationDate column is completely unnecessary. The simulation date and time is already written to the session.txt file in the output folder, and hisafer reads this into hop$metadata. Having this extra column just adds more things that have to read and stored in hisafer, thereby dragging memory, etc. SO, I have decided to have hisafer THROW OUT this column when reading the output profiles. Doing this has the added benefit that reading simulations from older versions of Hi-sAFe will look that same as reading simulations form the new version. Therefore, then can be merged via hop_merge() without any issue.

kevinwolz commented 4 years ago

@LECOMTE-ISADEV, regarding the new "trueyear" column in the weather file....

You said that the column is not optional. Okay, but what if I don't have special values for this column. Can I put "NA" in the column? Or will the model throw an error if the column does not contain numeric values?

mariegosme commented 4 years ago

I agree with Kevin, that adding the simulation date to all profiles is unnecessary, because now a given simulation folder is self-contained with all its inputs and outputs, and contains the session.txt file with this information, so no need to repeat it in each line of each profile ! The true YR column is much more usefull, as long as we agree before-hand as to the meanings of the codes used in this variable. Actually, ideally this could be a text variable so that we are free to explain in more or less detail how the data was obtained or generated (e.g "lavalette", "restinclieres A2", "safran 65000 1618000", "A1B_hadcm3q0_lat_43.8876_lon_3.8669"). but if it's not possible, we have to have codes. I would suggest: 1 = yes (true year, i.e. really measured in whatever location is given in the name of the file (if the name of the file is explicit), 2= yes, except water table, which is estimated (or should we consider that water table is never measured at daily timestep so actually code 1 would never exist and so this code 2 could become code 1). but once again, this could be unnecessary, if people gave meaningfull names to the file (e.g. observed_in_restinclieres_A2_until_20200631_then_A1B_hadcm3q0.wth, or maybe hisafer could propose to have a "comment" argument to define_hisafe, so that one could add this kind of information in the comment, and the comment could be added as a text file to the "support" folder, and then be read into the metadata of the hop?

LECOMTE-ISADEV commented 4 years ago

Christian seems to be very happy to have this new date column for excel analysis. Maybe we can keep it in export file but not in R hop ?

kevinwolz commented 4 years ago

@LECOMTE-ISADEV yes I think that's fine. My only concern is that as we continue to add more and more variables to the input and output, the simulations will just continue to get slower and slower! This one column is not a big deal, but there are definitely better ways to do this in the future.

The trueyear column is not even used by the model. Can we make this column OPTIONAL? In fact, can we make any columns beyond column #13 OPTIONAL? This way, if someone wants to do some crazy documentation as @mariegosme is suggesting, they could just add 5 columns to track how they are processing their weather data. Can you set Hi-sAFe to just read the first 13 columns and ignore any additional columns? If this is done, then we would not have to update any old wth files as well.

LECOMTE-ISADEV commented 4 years ago

I have correct some species format that was wrong (last line vari cropSpecies.zip ety parameters)

LECOMTE-ISADEV commented 4 years ago

I think in capsis it is not possible to define optional column in input files records ... i will ask françois

kevinwolz commented 4 years ago

Capsis will kill us all.

kevinwolz commented 4 years ago

@LECOMTE-ISADEV I don't understand what you updated in the plt files. They all look identical to the files that I already have.

LECOMTE-ISADEV commented 4 years ago

I have changed the last line in some wheat files like 1 Orjaune 789 411 1622 10. 551 0.0485 -0.5209 0.11 199 11 1.0 575 362 26625 584 0.0 0.0 it was wrong and make the model stop

kevinwolz commented 3 years ago

Still need to add definitions and some units to output_defs.txt but otherwise the updates should be all good to go on the hisafe4.1 branch. Now, we just need to TEST the updates with the new version of Hi-sAFe.

kevinwolz commented 3 years ago

@mariegosme I cannot run Hi-sAFe 4.1 on my mac yet. Can you? If so, would you be able to install the current version of hisafer from the hisafe4.1 branch on GitHub and do some testing based on the updates I have made (see top of this thread)?

LECOMTE-ISADEV commented 3 years ago

I did that yesterday and it is working perfectly. I have updated the 2 files input_defs and output_defs on my computer. I also updated the crop species plt files to ass the new cropHarmonicWeightedMean = 1. I did the same for tree species. I only have problems with some plot function but I think maybe I'm not very used to hisafeR :-)

LECOMTE-ISADEV commented 3 years ago

here is the extdata folder with my update. We have less export profiles now, because it was to much files.... now we only have annual Plot - plot - annualCells - monthCells - cells - annualTrees - trees - voxels - voxelsDetail - climate - I check all the output_defs variables names and unit, it was long but i didi IT !!!! Even for the plot !!!! Maybe the other profiles like plotDetail - cellsDetail - monthCellsDetail - are not good anymore, so maybe it would be better to delete them. I hope it is not a big work for you to delete these profile in hisafeR code ?? If you prefer for the calibration you can keep the older ones and we will upgrade this part later. extdata.zip

kevinwolz commented 3 years ago

@LECOMTE-ISADEV I am comparing your extdata with mine. Why did you delete profileNames and exportFrequencies from the input_defs?

LECOMTE-ISADEV commented 3 years ago

Sorry it is a mistake. I began to update for my my export version and revert... I forgot to replace theses 2 lines

kevinwolz commented 3 years ago

Okay got it.

I think we are still missing the text definitions for these variables:

cells teta tetsen slrac cumlr cumlracz supres ep

voxels lracz rljour drl lracsenz fhumirac waterStockTurfac waterStockSenfac

Can you help me with these? You can just tell me here and I will update the output_defs.txt

LECOMTE-ISADEV commented 3 years ago

You can remove that.... it was only for debugging

kevinwolz commented 3 years ago

Well I will leave them in the files in case someone wants to know they are available. We can deal with the definitions another time.