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

Updates for version 1.3 #33

Merged
merged 31 commits into from
Jan 8, 2023
Merged

Updates for version 1.3 #33

merged 31 commits into from
Jan 8, 2023

Conversation

kanishkan91
Copy link
Collaborator

@kanishkan91 kanishkan91 commented Oct 10, 2022

  1. Take out transition code to increase memory allocation
  2. Add parallelization for land type kernel density processing
  3. Add script to write ncdfs using xarray based on inputs in config file
  4. Add testing script for new users
  5. Make reading in region, basin mapping file parameterzied so users can read in their own region, basin mapping file
  6. Changes to code cov yml along with addition of badge.
  7. Changes to setup.py (New package dependencies, bumping up python version)
  8. Addition of demeter 1.3.1 workflow which accounts for all changes above.
  9. Adding new docs generated using sphinx

1. Take out transition code to increase memory allocation
2. Add parallelization for land type kernel density processing
3. Add post processing script to generate ncdf files corresponding to demeter runs with each LC as a subdata type
4. Add testing script for new users
@kanishkan91 kanishkan91 requested a review from crvernon October 10, 2022 19:10
@kanishkan91 kanishkan91 marked this pull request as draft October 10, 2022 19:10
1. Assign threads dynamically when applying convultion filter
2. Add ncdf_conversion to native demeter code base
3. Update mapping files for IM3 CLM team
4. Add xarray to setup
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2022

Codecov Report

Merging #33 (0ef1c0c) into main (90e4edf) will decrease coverage by 3.47%.
The diff coverage is 43.42%.

@@            Coverage Diff             @@
##             main      #33      +/-   ##
==========================================
- Coverage   73.49%   70.02%   -3.48%     
==========================================
  Files          17       18       +1     
  Lines        1343     1461     +118     
==========================================
+ Hits          987     1023      +36     
- Misses        356      438      +82     
Impacted Files Coverage Δ
demeter/demeter_io/reader.py 70.83% <ø> (ø)
setup.py 0.00% <ø> (ø)
demeter/ncdf_conversion.py 12.90% <12.90%> (ø)
demeter/demeter_io/writer.py 16.78% <71.42%> (+2.18%) ⬆️
demeter/config_reader.py 67.22% <83.33%> (-0.17%) ⬇️
demeter/process.py 98.24% <90.90%> (+7.17%) ⬆️
demeter/change/expansion.py 90.47% <100.00%> (-0.09%) ⬇️
demeter/change/intensification.py 88.70% <100.00%> (-0.10%) ⬇️
demeter/install_supplement.py 40.00% <100.00%> (ø)
demeter/preprocess_data.py 91.93% <100.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kanishkan91 kanishkan91 marked this pull request as ready for review October 18, 2022 00:18
Copy link

@marideeweber marideeweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a python amateur, I was able to successfully run demeter and view the outputs, as well as modify the spatial resolution and run it again. There were hiccups at the beginning due to package version issues, but those have been resolved by changing which package versions are required. Great work!

Copy link
Member

@crvernon crvernon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @kanishkan91! Just a few inline comments to see to.

Also, for the test data: let's stay away from pickled files and use parquet files instead. Pickled files carry version dependencies.

Thanks!

return ds


if __name__ == "__main__":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this code will only run from the target script, let's remove the direct call since we are in a package. So everything after 261 should go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!

@@ -57,7 +59,7 @@ def prep_step(self):
self.s.weights, self.s.spat_ludataharm)

# create transition array to store data
self.transitions = np.zeros(shape=(self.l_spat_region, self.l_order_rules, self.l_order_rules))
#self.transitions = np.zeros(shape=(self.l_spat_region, self.l_order_rules, self.l_order_rules))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's get rid of the commented code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!


self.config.logger.info("Saving land cover transition files for time step {0}...".format(self.step))
# self.config.logger.info("Saving land cover transition files for time step {0}...".format(self.step))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!


wdr.write_transitions(self.s, self.config, self.step, self.transitions)
# wdr.write_transitions(self.s, self.config, self.step, self.transitions)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!

return wdr.lc_timestep_csv(self.config, self.step, self.s.final_landclasses, self.s.spat_coords, orig_spat_aez,
self.s.spat_region, self.s.spat_water, self.s.cellarea, self.s.spat_ludataharm,
self.config.metric, self.config.tabular_units, self.write_outputs)
self.config.metric, self.config.tabular_units, self.write_outputs,write_ncdf,self.sce,self.res,write_csv,self.regrid_res)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to conform to Python's PEP8 style req, leave space after commas. E.g., write_ncdf, self.sce

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!

@@ -0,0 +1,97 @@

import xarray as x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import xarray using it's well known alias to avoid confusion: import xarray as xr

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file itself is redundant since this has been functionalized. I'l take this out.

@@ -0,0 +1,97 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import order of packages should follow PEP 8 style requirements (https://peps.python.org/pep-0008/#imports)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file itself is redundant since this has been functionalized. I'l take this out.

@@ -0,0 +1,9 @@
import demeter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks

setup.py Outdated
@@ -24,6 +24,7 @@ def readme():
'pandas~=1.2.4',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your requirements file has these versions pinned e.g., ==, I prefer this method where >= is used if possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!

@@ -31,6 +31,7 @@ jobs:
pip install pandas==1.2.4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment about pinning in setup.py file review

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!

@kanishkan91 kanishkan91 temporarily deployed to github-pages January 6, 2023 07:50 — with GitHub Pages Inactive
@kanishkan91 kanishkan91 temporarily deployed to github-pages January 6, 2023 10:17 — with GitHub Pages Inactive
@kanishkan91 kanishkan91 temporarily deployed to github-pages January 6, 2023 10:22 — with GitHub Pages Inactive
@kanishkan91 kanishkan91 temporarily deployed to github-pages January 6, 2023 10:23 — with GitHub Pages Inactive
@kanishkan91 kanishkan91 temporarily deployed to github-pages January 6, 2023 10:36 — with GitHub Pages Inactive
@kanishkan91 kanishkan91 temporarily deployed to github-pages January 8, 2023 04:04 — with GitHub Pages Inactive
@kanishkan91 kanishkan91 merged commit 9aeac52 into main Jan 8, 2023
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.

4 participants