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

base_demand, demand_pattern and demand_category at junctions go to/from gis #447

Merged
merged 13 commits into from
Nov 13, 2024

Conversation

angusmcb
Copy link
Contributor

Would be very happy to have your feedback on this :-)

Summary

As described in issue #434

Tests and documentation

Added a new test.

Acknowledgement

By contributing to this software project, I acknowledge that I have reviewed the software quality assurance guidelines and that my contributions are submitted under the Revised BSD License.

@angusmcb
Copy link
Contributor Author

Tests have not passed but I think they will once #445 is merged.

@kbonney kbonney self-requested a review October 1, 2024 19:54
@kbonney
Copy link
Collaborator

kbonney commented Oct 1, 2024

@angusmcb thanks for the PR. I got the tests to run properly by merging in the main branch, but now it looks like the doctests are failing. Since the GIS dataframes have new column names the outputs don't match the expected values in the docs.

@coveralls
Copy link

Coverage Status

coverage: 84.231% (+0.004%) from 84.227%
when pulling 80f6b69 on angusmcb:demands-in-gis
into 576dc01 on USEPA:main.

Copy link
Collaborator

@kbonney kbonney left a comment

Choose a reason for hiding this comment

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

These updates make sense. I approve. I will wait for #452 before pulling in as we will likely need to fix doctest issues again after that.

"""
if len(self.demand_timeseries_list) > 0:
return self.demand_timeseries_list[0].category
return 0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering what makes the most sense for the default return values. I need to check what the simulators default to when the demand_timeseries_list is empty.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The add_junction function uses None as the default for demand_pattern and demand_category, so we should follow that here.

@kaklise kaklise merged commit 104fc69 into USEPA:main Nov 13, 2024
41 checks passed
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