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

Issue 117 Merge private CollectionCost into public CollectionCost module #128

Merged

Conversation

parangat94
Copy link
Contributor

Ready for review.

@parangat94 parangat94 requested review from akey7 and eberlea March 10, 2020 19:26
Copy link
Contributor

@eberlea eberlea left a comment

Choose a reason for hiding this comment

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

@parangat94, please review my questions, check the code, and check the validation data, and then send back to me for review. It doesn't look like there are any changes to the utility results so that's good but there are some enormous differences between the validation data and the model result for the DW cases.
image

landbosse/model/CollectionCost.py Show resolved Hide resolved
landbosse/model/CollectionCost.py Show resolved Hide resolved
landbosse/model/CollectionCost.py Show resolved Hide resolved
landbosse/model/CollectionCost.py Show resolved Hide resolved
landbosse/tests/model/test_CollectionCost.py Show resolved Hide resolved
@akey7
Copy link
Contributor

akey7 commented Apr 1, 2020

@parangat94 Has there been any movement ahead on @eberlea comments? Once those changes are made and @eberlea approves, I will do my review.

@parangat94
Copy link
Contributor Author

parangat94 commented Apr 9, 2020

@eberlea @akey7 ready for review. I've made changes per your requests and re-ran the validation. Please see results below:

Important: Please note there is new expected validation data on the box. I made some minor tweaks to the expected collection cost breakdown (which were showing some minor discrepancies in expected value due to legacy bugs)

@eberlea
Copy link
Contributor

eberlea commented Apr 9, 2020

I'm reviewing this now. I've flagged some additional comments that need to be addressed before merging. I'll let you know when I'm done reviewing and it's ready for you to make final changes before merging.

@eberlea eberlea self-requested a review April 9, 2020 17:48
Copy link
Contributor

@eberlea eberlea left a comment

Choose a reason for hiding this comment

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

I think this is close. Validation passes. :-) Please address my one remaining comment below before you merge.

Copy link
Contributor

@akey7 akey7 left a comment

Choose a reason for hiding this comment

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

Waiting for the hardcoded value of 137 to be addressed in the conversation on CollectionCost.py

@parangat94
Copy link
Contributor Author

@akey7 merged develop into branch issue_108_integrate_DW, followed by merge of issue_108_integrate_DW into current branch (issue_117_integrate_DW_collection).

PR ready for final review.

Copy link
Contributor

@akey7 akey7 left a comment

Choose a reason for hiding this comment

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

Validation works. Looks good to me!

@parangat94 parangat94 requested review from eberlea and removed request for eberlea May 8, 2020 20:32
@parangat94 parangat94 merged commit 0fce2ca into issue_108_integrate_DW May 8, 2020
@parangat94 parangat94 deleted the issue_117_integrate_DW_collection branch May 8, 2020 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants