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

Add initial new GreenHEART (ODIES) documentation #72

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

johnjasa
Copy link
Collaborator

Add initial new GreenHEART (ODIES) documentation

This PR adds the first bit of documentation focused on the development of new new GreenHEART (ODIES).
Rather than having this info off in my sandbox repo, I moved the initial docs to here so development can more easily happen in one place.

PR Checklist

  • CHANGELOG.md has been updated to describe the changes made in this PR
  • Documentation
    • [-] Docstrings are up-to-date
    • Related docs/ files are up-to-date, or added when necessary
    • Documentation has been rebuilt successfully
    • [-] Examples have been updated
  • Tests pass (If not, and this is expected, please elaborate in the tests section)
  • PR description thoroughly describes the new feature, bug fix, etc.

Related issues

N/A

Impacted areas of the software

N/A

Additional supporting information

N/A

Test results, if applicable

N/A

RHammond2 and others added 7 commits November 1, 2024 17:00
* update PR template to be a bit more relevant

* udpate the bug report template

* update the feature request and replace duplicated text in the bug report

* update formatting so all descriptions are html comments

---------

Co-authored-by: John Jasa <[email protected]>
Copy link
Collaborator

@RHammond2 RHammond2 left a comment

Choose a reason for hiding this comment

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

Thanks for compiling all the thoughts on ODIES/new GreenHEART! I left a couple of comments on typos and small things like that, and some questions that may be out of scope for this PR that were at least worth raising.

@@ -5,22 +5,16 @@ on: [ push, pull_request ]
jobs:
build:
runs-on: ubuntu-latest
defaults:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why we're abandoning the environment settings? I don't have a strong preference either way, but was curious.

Comment on lines +16 to +17
- file: why_make_new_greenheart
- file: features_not_currently_implemented
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love that your voice is even included in the file naming!

@@ -0,0 +1,23 @@
# Class structure in ODIES

A major focus of ODIES is modularizing the components and system architecture so it's easier to construct and analyze complex hybrid power plants producing commodities for a variety of uses.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know line length isn't enforced in GreenHEART, but it would be nice if we avoided very long lines to be kinder to our future selves.

This would override any plant level financial model, and is useful for technologies that have unique financial considerations.
This will also be more relevant as we develop non-single-owner capabilities.

## Control model (optional, unused)
Copy link
Collaborator

Choose a reason for hiding this comment

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

A general comment is to watch out for defining undefined functionality that can come back to haunt us.


### Test location

Each folder within the `ODIES` package should have a corresponding `tests` folder.
Copy link
Collaborator

Choose a reason for hiding this comment

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

General comment that may not apply here: Should the unit, integration, and regression get separate folders within tests/ that then roughly or directly mimic the code structure within? This then allows for easily setting up a workflow for running the each individually, such as pytest --unit (https://github.com/NREL/OpenOA/blob/main/test/conftest.py#L15-L37).

I could certainly see unit/ following this guidance on mimicking the structure, but regression and integration probably might be more focused on workflows than smaller units of testing.


Historically, ODIES has been a sort of hybrid itself, where it contains both the tool itself as well as project-specific code.
Additionally, HOPP has been a separate tool that ODIES calls to obtain electricity production data and has been developed alongside ODIES.
This has led to a somewhat disjoint codebase that is difficult to maintain and understand.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"disjointed"

@@ -69,7 +69,7 @@ def test_system_power_report(self):
)
comp.compressor_power()
_motor_rating, total_system_power = comp.compressor_system_power()
assert total_system_power == approx(3627.3907562149357)
assert total_system_power == 3627.3907562149357
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now, who's got the errant change! I'm not sure how this one snuck in here too.

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.

2 participants