NREL / reV

Renewable Energy Potential (reV) Model
https://nrel.github.io/reV/
BSD 3-Clause "New" or "Revised" License
107 stars 24 forks source link

reV on GAPs #415

Closed ppinchuk closed 1 year ago

ppinchuk commented 1 year ago

This update contains quite a few changes, mostly to the reV CLI. Primarily, all CLI functionality has been moved to GAPs. This means a lot of the reV code having to do with the CLI has been deprecated.

Most importantly, a big documentation overhaul has been included.

This update also includes fixes for a few outstanding issues.

Since GAPs is not available on the platform, conda support is dropped with this update.

reVX tests will fail due to breaking API changes. Will update reVX code to confirm to new changes once this PR is merged.

ppinchuk commented 1 year ago

Ok @grantbuster, take a peek at the new Gen class. In particular, note that reV_run is now an instance method instead of a class method. This allows the docstring for the parameters to be broken up between the instance method and the class init. All parameters only have to be documented in the one place where they are pulled into the object. Note the new way of using the Gen class

gen = Gen(tech, points, sam_configs, res_file, lr_res_file=None,
          output_request=('cf_mean',), site_data=None, curtailment=None,
          gid_map=None,  sites_per_worker=None,
          mem_util_lim=0.4, scale_outputs=True, write_mapped_gids=False,
          bias_correct=None)
gen.reV_run(max_workers=1, pool_size=(os.cpu_count() * 2), timeout=1800)

does resemble the old class method implantation:

gen = Gen.reV_run(tech, points, sam_configs, res_file, lr_res_file=None,
                  output_request=('cf_mean',), site_data=None, curtailment=None,
                  gid_map=None, max_workers=1, sites_per_worker=None,
                  pool_size=(os.cpu_count() * 2), timeout=1800,
                  points_range=None, out_fpath=None, mem_util_lim=0.4,
                  scale_outputs=True, write_mapped_gids=False,
                  bias_correct=None)

No docstrings have actually been updated yet, I have focused purely on the code logic for now.

Let me know if you see any major red flags or funky things you'd like me to address.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 70.87% and project coverage change: +5.33% :tada:

Comparison is base (3417818) 81.60% compared to head (df8f4ff) 86.93%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #415 +/- ## ========================================== + Coverage 81.60% 86.93% +5.33% ========================================== Files 144 122 -22 Lines 21161 16814 -4347 ========================================== - Hits 17268 14617 -2651 + Misses 3893 2197 -1696 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `86.93% <70.87%> (+5.33%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files Changed](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL) | Coverage Δ | | |---|---|---| | [reV/SAM/SAM.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL1NBTS9TQU0ucHk=) | `85.45% <ø> (ø)` | | | [reV/bespoke/place\_turbines.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL2Jlc3Bva2UvcGxhY2VfdHVyYmluZXMucHk=) | `96.06% <ø> (ø)` | | | [reV/bespoke/plotting\_functions.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL2Jlc3Bva2UvcGxvdHRpbmdfZnVuY3Rpb25zLnB5) | `17.80% <ø> (ø)` | | | [reV/config/base\_analysis\_config.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL2NvbmZpZy9iYXNlX2FuYWx5c2lzX2NvbmZpZy5weQ==) | `34.37% <ø> (-46.88%)` | :arrow_down: | | [reV/config/execution.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL2NvbmZpZy9leGVjdXRpb24ucHk=) | `55.73% <ø> (-14.76%)` | :arrow_down: | | [reV/handlers/\_\_init\_\_.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL2hhbmRsZXJzL19faW5pdF9fLnB5) | `100.00% <ø> (ø)` | | | [reV/losses/power\_curve.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL2xvc3Nlcy9wb3dlcl9jdXJ2ZS5weQ==) | `94.37% <ø> (+0.03%)` | :arrow_up: | | [reV/losses/scheduled.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL2xvc3Nlcy9zY2hlZHVsZWQucHk=) | `100.00% <ø> (ø)` | | | [reV/losses/utils.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL2xvc3Nlcy91dGlscy5weQ==) | `96.29% <ø> (ø)` | | | [reV/supply\_curve/sc\_aggregation.py](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL#diff-cmVWL3N1cHBseV9jdXJ2ZS9zY19hZ2dyZWdhdGlvbi5weQ==) | `81.58% <ø> (+1.45%)` | :arrow_up: | | ... and [72 more](https://app.codecov.io/gh/NREL/reV/pull/415?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL) | | ... and [5 files with indirect coverage changes](https://app.codecov.io/gh/NREL/reV/pull/415/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=NREL)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ppinchuk commented 1 year ago

All great questions!

  1. I plan on removing all existing logic in reV/generation/cli_gen.py completely. There is still some CLI-specific logic that I was thinking about moving to reV/generation/cli_gen.py, but I don't really have strong feelings about that. I would be happy to keep that in the generation file if you think that is most appropriate.
  2. I do plan on removing most of the config classes. I definitely plan on deprecating the generation config and other command configs. Some configs do seem to have a use though (e.g. Curtailment config). I was planning on re-evaluating their structure towards the end of the refactor to see if they should remain as-is or if there are ways to simplify them.
  3. Yes, definitely. I like the Gen.reV_run() -> Gen.run() change a lot so I would be perfectly happy to do that.
  4. Yes definitely. I will send you a local copy of them so that you can explore, but I would also be happy to jump on a call and discuss.
grantbuster commented 1 year ago

All great questions!

  1. I plan on removing all existing logic in reV/generation/cli_gen.py completely. There is still some CLI-specific logic that I was thinking about moving to reV/generation/cli_gen.py, but I don't really have strong feelings about that. I would be happy to keep that in the generation file if you think that is most appropriate.
  2. I do plan on removing most of the config classes. I definitely plan on deprecating the generation config and other command configs. Some configs do seem to have a use though (e.g. Curtailment config). I was planning on re-evaluating their structure towards the end of the refactor to see if they should remain as-is or if there are ways to simplify them.
  3. Yes, definitely. I like the Gen.reV_run() -> Gen.run() change a lot so I would be perfectly happy to do that.
  4. Yes definitely. I will send you a local copy of them so that you can explore, but I would also be happy to jump on a call and discuss.

Okay great!

  1. No preference on where that goes but i fully support removing as much code as possible which it sounds like you're doing
  2. It might be useful to keep the base configs around at the very least but besides that i don't have strong opinions.
  3. Great
  4. Great
  5. Can people can still run a local job (no Eagle) via the cli?
ppinchuk commented 1 year ago

Noted on all of those. And yes, people can still totally run the commands locally (but they will need to use a config)

grantbuster commented 1 year ago

Noted on all of those. And yes, people can still totally run the commands locally (but they will need to use a config)

Yep got it. and can run from python if they want to. PERFECT.