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

Refactor parameterisation #28

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

Conversation

christian34
Copy link
Contributor

@pradal @chrlecarpentier @Emblanc Here is my proposal for clarifying parameterisation / initialisation #26
I split parameters in modules: parameter.py (default pars), genotypic_parameters.py and experimental conditions.py.
The initialisation of parameters is done in parameter.py as a dict and imported as global variables in lpy
I also isolate the crop_conception function in a new module
The interface for parameter setting in csv is the same as before (same parameter names, including genotype dependant parameters) except or one of them : location now refer to the location / latitude only and I add meteo to refers to the climatic data. Experimental_condition.py handle that properly.

@pradal
Copy link
Contributor

pradal commented Dec 20, 2017

@christian34 Nice job guy. For me it is much more simpler and easier to read.

@christian34
Copy link
Contributor Author

The PR is now updated with changes brought by PR #20, #21, #22 and #25. I also rebase it on top of master

@pradal
Copy link
Contributor

pradal commented Apr 4, 2018

Do not understand what is the status. Can you clarify?

@christian34
Copy link
Contributor Author

The status is 'ready to merge'.
As this PR moves all the global variables initialisation in external modules, all changes in variable initialisation done directly in walter.lpy and merged since this PR was open (PR #20, #21, #22 and #25) had to be reported in the external modules before solving conflicts.

@pradal
Copy link
Contributor

pradal commented Apr 10, 2018

It would be good to merge the branch before the student began to work on the refactoring.

@Emblanc
Copy link
Collaborator

Emblanc commented Apr 12, 2018

I agree : Bilâl should work on a version of the model that include this PR.
But at this stage of the development of WALTer, I think it would be best to have 2 branches. We need to have a stabilized branch to run the simulations of my PhD. There also should be a development branch on which Bilâl can work.
For the first branch, the goal is to have a version of WALTer without major bugs, to run the simulations for my PhD as soon as possible. Once it is ready, this branch should not evolve, as I must use the same version of the model for all my analysis.
For the second branch, the goal is to develop a modular version of WALTer. I think this PR should be merged in the second branch.

@pradal
Copy link
Contributor

pradal commented Apr 12, 2018

OK. As soon as you have a working version, I recommend to make a tag that will fix the version. With bilal, we can work on a branch named develop.

@christian34
Copy link
Contributor Author

Okay for converging on a debugged/stabilised master. Lets just remember that all changes accepted in walter.lpy will have to be manually reported to this branch (like I just did). It may be good to synchronise master and develop from time to time to avoid this. My idea would be to merge this PR in master too once bilal has implemented his first global non-regression test (ie check that all outputs are the same as in master).

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.

3 participants