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

Jdh/update docs #155

Merged
merged 4 commits into from
Apr 20, 2023
Merged

Jdh/update docs #155

merged 4 commits into from
Apr 20, 2023

Conversation

jhoshiko
Copy link
Collaborator

This PR contains the following:

  • updates docs with expected file types
  • removes all config/doc references to geofences.
  • removes all config/doc references to demand forecast files
  • removes demand forecast files from scenarios
  • updated vehicle section in the inputs docs to correctly display the vehicles.csv schema

Looks like this also closes #111

If there are any additional changes to the docs that we think would be beneficial, please let me know!

@jhoshiko jhoshiko requested review from nreinicke and a user April 19, 2023 22:25
@jhoshiko jhoshiko marked this pull request as draft April 19, 2023 22:38
@jhoshiko
Copy link
Collaborator Author

Hmm, looks like some of our unit tests are still reliant on the Denver demand forecast files. I can update the unit tests or restore the demand forecast files. Let me know what you prefer.

I also noticed references to demand forecast files in both nrel/hive/dispatcher/forecaster/basic_forecaster.py and nrel/hive/config/input.py. Looks like we could remove references to demand forecast files from input.py, and we can leave basic_forecaster.py alone since nothing is dependent on it. Let me know what you think!

@nreinicke
Copy link
Collaborator

I think it would make sense to remove the demand forecast file from the input config at the very least. I would also be open to pulling out the forecaster module but that could be deferred.

@robfitzgerald
Copy link
Collaborator

right. in general, i think this is the strategy:

  • if there are tests testing demand forecasting, we can remove them
  • if any tests programmatically load hive with a demand forecast, remove the addition of the forecast
  • if any scenario yaml files reference a forecast, remove the reference

for today, we could probably target the least amount of refactoring to get the tests working again, and make it a future task to really clean up all references to forecasting. does that sound right?

forecasting is a useful thingie but it really shouldn't be hive's problem. it can instead just a property of a custom InstructionGenerator to load and incorporate any forecasting.

…ile from mock config in mock lobster, removed default forecast file key pair in default hive config
@jhoshiko jhoshiko marked this pull request as ready for review April 20, 2023 21:19
@jhoshiko
Copy link
Collaborator Author

jhoshiko commented Apr 20, 2023

Alright, this should be ready for review. I've left the forecaster module alone, but I've removed demand_forecast_file from input.py and the mock config in mock lobster. If you have any additional change requests, please let me know!

note: I've left one reference to the demand forecast file and geofence in the example log outputs in the README. I figured we should just update the entire log and that we can have that be a good first issue for someone at pycon.

Copy link
Collaborator

@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

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

looks good. thanks for removing those references, they can be distracting when we are introducing people to the model. as for geofence, i have this idea that we could come back to geofences as a special case of fleet id generation (a membership relationship is another way of describing geofencing and probably faster to compute under-the-hood too).

ok pycon's gonna be a party now fo sho!

@jhoshiko jhoshiko merged commit 2220709 into main Apr 20, 2023
@jhoshiko jhoshiko deleted the jdh/update-docs branch April 20, 2023 22:45
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.

Vehicle input description describes Station
3 participants