Skip to content

add offgrid wind to job/ app, update generator and PV defaults#343

Merged
adfarth merged 29 commits into
developfrom
wind_off_grid
Aug 25, 2022
Merged

add offgrid wind to job/ app, update generator and PV defaults#343
adfarth merged 29 commits into
developfrom
wind_off_grid

Conversation

@adfarth
Copy link
Copy Markdown
Collaborator

@adfarth adfarth commented Aug 1, 2022

Please check if the PR fulfills these requirements

  • CHANGELOG.md is updated
  • Tests for the changes have been added (for bug fixes / features)
  • [ N/A] Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

(Bug fix, feature, docs update, ...)

The following Minor Updates have been added to the job/ endpoint:

Added
  • job/ endpoint: Add inputs and validation to model off-grid wind
Changed
  • job/models.py: remove Generator fuel_slope_gal_per_kwh and fuel_intercept_gal_per_hr defaults based on size, keep defaults independent of size
  • job/validators.py: Align PV tilt and aziumth defaults with API v2 behavior, based on location and PV type

What is the current behavior?

(You can also link to an open issue here)

  • Cannot model wind when off_grid_flag=True
  • Generator fuel intercept and slope defaults set dependent upon size
  • PV tilt and azimuth defaults do not reflect API v2 behavior

What is the new behavior (if this is a feature change)?

See above

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)

The PV and generator default changes are breaking as compared to previous v3 (job/ app) behavior, but not as compared to v2.

@adfarth
Copy link
Copy Markdown
Collaborator Author

adfarth commented Aug 1, 2022

@rathod-b do we need to use a dummy default tilt for PV in validators.py here? Could we not set null=True in models.py and update in validators as we do for other inputs (e.g. operating reserves required)?

@adfarth adfarth changed the title Add offgrid wind to v3 add offgrid wind to job/ app, change generator defaults Aug 1, 2022
adfarth added 2 commits August 1, 2022 16:04
changing defaults to 0.076 gal/kWh and 0 gal/hr intercept. This aligns with current API (v2) and Julia Pkg defaults. Remove default slope and intercept based on system size.
@adfarth
Copy link
Copy Markdown
Collaborator Author

adfarth commented Aug 1, 2022

@rathod-b do we need to provide this message in msg_dict["info"] every time off_grid_flag is true?:

            if "ElectricTariff" in self.models.keys():
                msg_dict["ignored inputs"] = ("ElectricTariff inputs are not applicable when off_grid_flag is true, and will be ignored. "
                                "Provided ElectricTariff can be removed from inputs")
            msg_dict["info"] = ("When off_grid_flag is true, only PV, ElectricStorage, Generator technologies can be modeled.")```

Could we just show it if a disallowed key is supplied, as we do in the Julia Pkg [here](https://github.com/NREL/REopt.jl/blob/8d7beb8a916d1cdf8571c268d44a6ef86dc07a7b/src/core/scenario.jl#L88)? 

Comment thread job/validators.py Outdated
Comment thread job/validators.py Outdated
Comment thread job/validators.py Outdated
Comment thread job/validators.py Outdated
@adfarth
Copy link
Copy Markdown
Collaborator Author

adfarth commented Aug 1, 2022

@rathod-b This is mostly for my own edification, but do we (and why?) need null=True in models.py if we are also providing a default value, such as for the offgrid_other_capital_costs here?

Comment thread job/validators.py Outdated
Comment thread job/validators.py Outdated
Comment thread job/validators.py Outdated
Comment thread CHANGELOG.md
Comment thread reo/nested_inputs.py
Comment thread job/validators.py Outdated
Comment thread job/validators.py Outdated
Comment thread job/validators.py Outdated
Comment thread job/validators.py
Comment thread job/validators.py
Comment thread job/test/test_validator.py Outdated
@hdunham
Copy link
Copy Markdown
Collaborator

hdunham commented Aug 4, 2022

I've finished reviewing and left some comments. The PR description also needs to be filled out.

rathod-b and others added 2 commits August 5, 2022 11:31
Simplify how we validate certain fields when off-grid is true
Comment thread job/validators.py Outdated
Comment thread job/validators.py Outdated
Comment thread job/validators.py
Comment thread job/validators.py
@hdunham
Copy link
Copy Markdown
Collaborator

hdunham commented Aug 8, 2022

@rathod-b This is mostly for my own edification, but do we (and why?) need null=True in models.py if we are also providing a default value, such as for the offgrid_other_capital_costs here?

Technically we can remove the null=True from fields like this because per my understanding, setting a default value means this field is not going to be null (unless the code fails). If we were to update the default value downstream in cross_clean() then we would remove the default here so the field can be set correctly in cross_clean (as we do with PV tilt now).

My understanding is also that null=True isn't necessary if a default is provided, though I haven't been able to find this said explicitly in the Django docs so had been holding off on the issue. Removing all unnecessary null=True is something that I've been keeping in mind as a low priority to-do for a while and was going to include in a general validator.py/models.py clean up after finishing more urgent capabilities.

@rathod-b
Copy link
Copy Markdown
Collaborator

@rathod-b This is mostly for my own edification, but do we (and why?) need null=True in models.py if we are also providing a default value, such as for the offgrid_other_capital_costs here?

Technically we can remove the null=True from fields like this because per my understanding, setting a default value means this field is not going to be null (unless the code fails). If we were to update the default value downstream in cross_clean() then we would remove the default here so the field can be set correctly in cross_clean (as we do with PV tilt now).

My understanding is also that null=True isn't necessary if a default is provided, though I haven't been able to find this said explicitly in the Django docs so had been holding off on the issue. Removing all unnecessary null=True is something that I've been keeping in mind as a low priority to-do for a while and was going to include in a general validator.py/models.py clean up after finishing more urgent capabilities.

Good idea, once we merge everything into API v3 over the next few weeks it would be nice to clean up the code and remove redundancies/make functions for repetitive validations in model cross_clean

@adfarth adfarth changed the title add offgrid wind to job/ app, change generator defaults add offgrid wind to job/ app, update generator and PV defaults Aug 11, 2022
Correct how defaults were being set for some off-grid related inputs, update default values, update the testcase, up REopt
@rathod-b rathod-b marked this pull request as ready for review August 11, 2022 18:03
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.

3 participants