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

Apply black to python directory #1577

Merged
merged 19 commits into from
Jul 5, 2022
Merged

Conversation

ekluzek
Copy link
Collaborator

@ekluzek ekluzek commented Dec 13, 2021

Description of changes

Run black on python directory, and add autorun of check with black for python directory.

Specific notes

Contributors other than yourself, if any:

CTSM Issues Fixed (include github issue #):
Fixes #1471

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any: python tests

@ekluzek ekluzek added the code health improving internal code structure to make easier to maintain (sustainability) label Dec 13, 2021
@ekluzek ekluzek self-assigned this Dec 13, 2021
@billsacks
Copy link
Member

billsacks commented Dec 13, 2021

@ekluzek since this will interfere with git blame pretty significantly, what would you think about adding a file like .git-blame-ignore-revs in the root of the ctsm repository listing the commit(s) that ran reformatting operations? See https://akrabat.com/ignoring-revisions-with-git-blame/ for details.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Dec 13, 2021

@billsacks that is a trick that I read about from the "black" documentation. I didn't know exactly what it meant, but in principle sounds like a really good idea. So I'll figure that out and add it to the PR.

@billsacks
Copy link
Member

As discussed in today's standup: plan is: after bringing in #1448 and upcoming PRs for subset_data.py, we'll be at a cleaner break point. Then @ekluzek will redo the black-ification of the python and we'll bring this in.

The general principle here is: If possible, we'll try to only need to do a full-rewriting run of black once (to avoid messing with git blame too much).

ekluzek added 3 commits March 9, 2022 17:30
Implement PCT_URBAN_MAX to minimize dynamic urban memory

Read in 'PCT_URBAN_MAX' from the landuse timeseries file (maximum urban
percentage throughout timeseries) and initialize urban landunits in
memory only where PCT_URBAN_MAX is greater than zero.
@ekluzek
Copy link
Collaborator Author

ekluzek commented Jun 2, 2022

This should be coordinated with #1768. We should run black (and any pre-commit hooks) we want in place on our code before we bring in pre-commit as a standard tool to use.

ekluzek added 11 commits June 6, 2022 11:15
Correct perched water table calculation

Modifies the calculation of the frost table and perched water table
layers.

This brings in the answer-changing aspects of the Hillslope Hydrology
branch (ESCOMP#1715).

Specific changes are:

* PerchedWaterTable

1) Frost table depth

a) original frost table determination looped from the top
of the soil column downward to the index of the first layer
whose temperature was <= freezing, and whose neighbor above
had a temperature above freezing.  The frost table depth is
then given by the node of that soil layer, i.e. z(k_frz).

b) in the new method, the same index is found, but the
depth of the frost table is given by the depth of the top
of the frozen layer, i.e. zi(k_frz-1).  Note zi(k_frz) would
be the bottom of layer k_frz.

2) Perched water table depth

a) in the original formulation, a loop from k_frz to layer 1
was used to identify the deepest layer in 1:k_frz whose
volumetric soil moisture was greater than a threshold given
by sat_lev (e.g. sat_lev = 0.9).

b) in the new method, the search is only done if k_frz is
greater than 1.  The rationale is that if k_frz = 1, then
zwt_perched has already been initialized to the frost_table
depth (which is equal to the top of the uppermost soil layer),
and therefore no search is required.

3) Determining perched water table depth within layer
identified by index k_perch in 2)

a) in the original formulation, the perched water table
depth was calculated by linearly interpolating between layers
k_perch and k_perch+1, with no consideration of their relative
values.  In the case where the deeper layer was drier than the
layer above it, this could result in values far outside the
soil layer.

b) in the new formulation, if the deeper layer is drier than the
layer above it (s1 > s2), then the perched water table depth is
simply given by the depth of the upper surface of layer k_perch,
i.e. zi(c,k_perch-1).

* PerchedLateralFlow

1) Removal of icefrac calculation

a) in the original calculation, the frozen layer was included,
so an ice impedance factor was calculated using icefrac.  If
only unfrozen layers are used, no ice impedance factor is needed.

b) in the new formulation, the icefrac variable and loop are
removed.

2) Move loop calculating frost and perched water table depths

a) in the original formulation, the frost and perched water
table depths were calculated in the same loop as the calculation
of the lateral flow from the perched saturated zone.

b) in preparation for the hillslope hydrology branch, this
calculation is moved into its own loop.

3) q_perch calculation

a)in the original formulation, q_perch was calculated by summing
over layers k_perch to k_frost.  However, because k_frost is now
identified as the frozen layer, and its depth the top of the frozen
layer, it should not be included in the calculation; only the
unfrozen layers above it should be included.

b) in the new formulation, the loop is bounded by k_frost-1 instead
of k_frost.

4) Removal of water from perched saturated zone

a) the in the original method, the frozen layer (k_frost)
was included in the loop.  Also, the drainage was defined to be
negative, which was confusing.

b) in the new method, the frozen layer is not included in the
loop; water is only removed from the unfrozen layers above k_frost.
Calculate drainage as positive values, which are then subtracted
from the soil moisture in each layer.

 Conflicts:
	python/ctsm/path_utils.py
…psf version as recommended by black in the 22.3.0 documentation on github actions
…he --diff option so just lists files that would change
This set of changes 1) allows FATES to dictate the number of patches on the naturaly vegetated land unit and 2) is compatible with a version of FATES that updates the parameter file format (changes in parameter naming and parameter set).  Efforts were made to adhere to the system of how patches are looped and loop bounds are defined. There is interaction with the reading of the surface file, which previously dictated all patch counts based on the number of PFTs in the file.  FATES-SP and biogeography preserves the ability to use surface file weighting factors associated with PFTs and CFTs to drive area fractions in FATES.  This change is motivated by FATES-side updates to the parameter file, which includes new settings where the user can control the number of primary "fates_maxpatch_primary" and secondary "fates_maxpatch_secondary" patches. Users should also be aware of a new namelist setting "use_fates_tree_damage" which defaults to false (inoperative).
@ekluzek ekluzek merged commit 9d32e27 into ESCOMP:master Jul 5, 2022
@ekluzek ekluzek deleted the apply_black_to_python branch July 5, 2022 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health improving internal code structure to make easier to maintain (sustainability)
Projects
Status: Done (non release/external)
Development

Successfully merging this pull request may close these issues.

Add support for using "black" as a python formatter to our python coding process
2 participants