-
Notifications
You must be signed in to change notification settings - Fork 15
Various winter improvements #242
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: main
Are you sure you want to change the base?
Conversation
New classes Parameter and ParameterSet to better manager parameters of the model
c321a2a to
a3d515f
Compare
be366a3 to
3c2eea8
Compare
c4fd94c to
72dd752
Compare
97fb3de to
f9e961b
Compare
5d7f05b to
1ae982f
Compare
f8d9118 to
b245677
Compare
* bug quickfix for small radiuses in get_prominent_cities * better legend in plot_od_flows (still have to improve it) * ability to save OD flows as an image (in local folder for now, to improve)
MultiSeedPopulationTrips with some test code
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #242 +/- ##
=======================================
Coverage 70.42% 70.42%
=======================================
Files 56 56
Lines 2424 2424
=======================================
Hits 1707 1707
Misses 717 717 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This 0.9999999 default value seems too high to me ? 0.99986 comes from the radiation model with selection paper from Simini (https://arxiv.org/pdf/1206.4359).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the value I found using tests, but we will need to run proper tests soon anyway on diverse territories, so we can discuss best value later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK we will have to validate this, because 0.9999999 probably means we remove all short ODs, because they will not be sampled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make a single PopulationTrips class with the option to have multiple seeds ? We probably should remove all the main as this should not run as a script anyway ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to discuss it! I hesitated for a long time indeed, here's why having a proper class was my option:
- We still hope to have good results with using a single seed, ie not only use cases will imply multiple seeds
- Using multiple seeds would mean we have to rewrite every function in PopulationTrips, which is a big change and creates a long file
- On the opposite, by having a MultiSeedPopulationTrips, we can have a clear class for that purpose, which uses PopulationsTrips as it is and keeps it stable. That also opens the door to MultiParamsPopulationTrips (one single seed but slightly different value of costs of time for example), which could be useful for future research
(Once we agree on the solution, we can obviously remove the "if name == "main":" part)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that single seed is just a special case of multiple seed ?
I wonder if we could modify the Asset class to handle parametric inputs automatically, so we don't need a special class every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k_mode_sequences=42 ? What's the idea here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a test, we can switch back to a lower value (but we'll need to test the sensibility to that soon)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think WorkplaceByResidenceCounts would be a better name ? INSEE does not really give flows. Maybe remove the fr suffix, create a generic class and then have methods for each country / source ? Like in https://github.com/mobility-team/mobility/blob/main/mobility/population.py. We should be able to use swiss Statent data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also avoid the name == "main" block as this should not run as a script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INSEE urls are unstable, so we should copy the files on data.gouv.fr first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with the comments, I will do these changes asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
25 seems huge, except if you want to exclude bicycle as a mode ? It means that cycling will get a 0 % probability of being selected for nearly all comparisons with the other modes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bicycle was really overrepresented otherwise. We'll need to investigate that in the near future!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good first version ! I think we can avoid the post_init / dict serialization. Should I take this in a separate PR ? I think it's substantial enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh oh!
There was an error while loading. Please reload this page.