Redoing PR for fixes #57 and #60 (and maybe #59) #62
Conversation
A different part of the code tried to access what used to be the distance matrix by indexing with a float. Fixes #57
|
I'll take care of this tomorrow morning. |
| @@ -255,11 +280,15 @@ def network(self): | |||
| return self._network | |||
|
|
|||
| @property | |||
| def original_metrics(self): | |||
| """returns the original (unprocessed) metrics data_frame""" | |||
| return self._original_metrics | |||
There was a problem hiding this comment.
Why is original_metrics a property and not just an attribute?
There was a problem hiding this comment.
to keep consistent with _network and _metrics. Better ideas welcome.
I don't like original_metrics being there at all, but the dependency in sequencer was hard to get rid of quickly. Will address that with large refactoring or rewrite.
There was a problem hiding this comment.
Oh yeah, now that you mention it there's a lot of property weirdness going on here... May as well clean it up in a different PR.
| nwp = NetworkPlan(network, metrics, prioritize='Population', proj='wgs4') | ||
| model = EnergyMaximizeReturn(nwp) | ||
| model.sequence() | ||
| #todo: check the result |
There was a problem hiding this comment.
Though this catches fix#57, we should make sure the results are valid in some way
There was a problem hiding this comment.
Could you check against a known-good result?
|
Is this a good and happy PR now? |
|
I think so. Let's keep in mind #64 (this basically needs an overhaul, so lots of nit-picking isn't productive). If you don't see anything glaring, I'm gonna do a test on some of the failed scenarios in modelrunner and then release. |
|
Ship it |
Closes #60, #59
Assigning to @vr2262 (as my trigger was too quick on merging my own PR last time)