Split pmodel file

[MarionBWeinzierl]

Fixes #58 . The pmodel module is split so that each class has it's own file. The docstrings and documentation are adapted accordingly.

@davidorme , could you double-check that I didn't miss or break anything in the docs?

[codecov-commenter]

Codecov Report

Merging #136 (7b3f870) into develop (e28d842) will increase coverage by 0.13%. The diff coverage is 90.21%.

@@             Coverage Diff             @@
##           develop     #136      +/-   ##
===========================================
+ Coverage    93.95%   94.09%   +0.13%     
===========================================
  Files           18       21       +3     
  Lines         1109     1134      +25     
===========================================
+ Hits          1042     1067      +25     
  Misses          67       67              

Files	Coverage Δ
pyrealm/constants/pmodel_const.py	100.00% <ø> (ø)
pyrealm/pmodel/__init__.py	100.00% <100.00%> (ø)
pyrealm/pmodel/functions.py	97.56% <ø> (ø)
pyrealm/pmodel/pmodel.py	93.20% <100.00%> (+2.93%)	:arrow_up:
pyrealm/pmodel/subdaily.py	95.28% <ø> (ø)
pyrealm/pmodel/jmax_limitation.py	90.74% <90.74%> (ø)
pyrealm/pmodel/pmodel_environment.py	85.71% <85.71%> (ø)
pyrealm/pmodel/calc_optimal_chi.py	90.80% <90.80%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[MarionBWeinzierl]

@MarionBWeinzierl That looks great, thanks.

The only question I have (and this is very fussy) is that PEP8 recommends module names be lower case? I do think its a bit clearer if CamelCase is reserved for classes. I'd probably make the modules: calc_optimal_chi.py, jmax_limitation.py, pmodel.py and pmodel_environment.py.

Thoughts?

Hm, good point. I had used camel case as each file contains now one class, so I took the files as classes. But it would probably be similarly valid to say they are modules containing one class.

[davidorme]

That makes a lot of sense, but at least one of them (CalcOptimalChi.py) is likely to expand to be provide an abstract base class and then some subclasses of it. I wouldn't rule out putting other small things in these files if it seems like an appropriate home.

[MarionBWeinzierl]

That makes a lot of sense, but at least one of them (CalcOptimalChi.py) is likely to expand to be provide an abstract base class and then some subclasses of it. I wouldn't rule out putting other small things in these files if it seems like an appropriate home.

OK, I made the changes

[MarionBWeinzierl]

Looks good to me. There are some places where we could leave original import paths, because the components are all imported into pmodel.__init__.py, but I don't think there's any advantage to using the flattened namespace over the original locations?

So for example from pyrealm.pmodel.pmodel_environment import PModelEnvironment could also stay as from pyrealm.pmodel import PModelEnvironment, but I don't think it matters.

Hi, ok, if you prefer that we can do that (I had gotten so many "not found" errors that I changed those to be on sure). Could you have a look at the "Files changed" tab and check whether I found all the chances, or whether I missed something/made a mistake?

[MarionBWeinzierl]

I asked colleagues, and basically got the answer "flattened namespaces are good, probably no performance issue" as well as "explicit is better than implicit". I made the decision to go back to the flattened namespace, but am now getting lots of error, presumable due to an issue like this: https://stackoverflow.com/questions/59762996/how-to-fix-attributeerror-partially-initialized-module , as pmodel is both the module name and a submodule/class name.

But didn't we have that situation before with pmodel?

[MarionBWeinzierl]

Ha. I resolved the problem by reordering the imports. Then I committed, and the precommits ran, and isort failed and changed the order back!

[davidorme]

Good old isort - still I don't think having the import process being sensitive to the ordering of the imports within a module sounds robust. I think that being explicit about the imports within the modules should fix that though? Then the module __init__ provides a flattened namespace but the submodules still point to the correct home?

[davidorme]

Before, all of the classes were imported from the same module, so that modules didn't need to contain imports for anything included in the flattened namespace.

[MarionBWeinzierl]

Yeah, that will fix it. So I'll revert what I've done today.