geoschem / integrated_methane_inversion

Integrated Methane Inversion workflow repository.
https://imi.readthedocs.org
MIT License
25 stars 19 forks source link

chore/component-variable-manifests: improve traceability of variables #159

Closed laestrada closed 11 months ago

laestrada commented 11 months ago

Name and Institution (Required)

Name: Lucas Estrada Institution: Harvard University

Describe the update

As pointed out in #158, the components make use of inherited variables from parent scripts. This can cause confusion in determining the origin of a variable. Here I generate header comments above component functions listing inherited variables and variables defined within the function.

@djvaron I leaned toward verbosity in this PR by defining both inherited and defined variables. Although I can see it becoming a chore to keep the defined variables up to date in future development. I personally find the inherited variables more confusing to track -- do you think we need both?

djvaron commented 11 months ago

Thanks for putting this together so quickly! I agree the inherited variables are the trickier ones to keep track of. So I could see only listing those ones, which would also make maintenance a bit easier. But on the other hand...

What do you think is the best way to track from where inherited variables are inherited? Do we need to figure out where a script is called and then look for defined/inherited variables there? I could see that being a headache if we don't list defined variables.

Either way, this seems like a good start.

Some general comments / ideas for the future:

laestrada commented 11 months ago

@djvaron

One should be able to infer where a variable is inherited by looking at the calling component of the function. If the parent component also inherited that variable, you look one level up (the grandparent component). But admittedly this can be confusing if the child function is called from multiple places.

I like the idea of a sanitizer (or linter) and single manifest file that gets updated, although I think using a python script would be better for crawling through the files and updating the manifest. We could even set it up to trigger a github action that automatically gets run on new pushes to the repository with an error if the manifest isn't updated.

I am going to update this PR and give the single manifest a go.