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.

Sign up for GitHub

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

Jump to bottom

Improve docstrings #34

Merged
merged 8 commits into from
Feb 27, 2023
Merged

Improve docstrings #34

merged 8 commits into from
Feb 27, 2023

Conversation

Mind-the-Cap
Copy link
Contributor

@Mind-the-Cap Mind-the-Cap commented Feb 17, 2023 •
edited

Improve docstrings and comments for

  • ENTD-2008 and EMP-2019 parsers
  • trip_sampler and safe_sample

Also:

  • give explicit default source name (EMP-2019)

@Mind-the-Cap
Improve docstrings for ENTD-2008 parser
a0d3160 
And also:
* improve a bit docstring for trip_sampler
* explicit default source name (EMP-2019)
@Mind-the-Cap Mind-the-Cap requested review from a team and removed request for a team February 17, 2023 16:28
@codecov
Copy link

codecov bot commented Feb 17, 2023 •
edited

Codecov Report

Merging #34 (c71eb27) into main (64718fc) will increase coverage by 0.07%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main      #34      +/-   ##
==========================================
+ Coverage   94.69%   94.76%   +0.07%     
==========================================
  Files          10       10              
  Lines         490      497       +7     
==========================================
+ Hits          464      471       +7     
  Misses         26       26
Impacted Files Coverage Δ
mobility/parsers/entd_2008.py 99.33% <ø> (ø)
mobility/get_survey_data.py 91.48% <33.33%> (-1.85%) ⬇️
mobility/parsers/emp_2019.py 99.47% <100.00%> (+<0.01%) ⬆️
mobility/safe_sample.py 90.00% <100.00%> (+1.11%) ⬆️
mobility/trip_sampler.py 88.23% <100.00%> (+1.47%) ⬆️
test/test_parsers.py 100.00% <100.00%> (ø)
test/test_trip_sampler.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Mind-the-Cap
Mind-the-Cap commented Feb 17, 2023
View reviewed changes
mobility/parsers/entd_2008.py
@@ -188,7 +227,7 @@ def prepare_entd_2008(proxies={}):
"V2_OLDACPA07",
"V2_OLDACPA08",
"V2_OLDACPA09",
"V2_OLDARCOM_UUCat",
"V2_OLDARCOM_UUCat", # Why UUCAT and not UU2010 like elsewhere?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On utilise une autre catégorie, alors que UU2010 me semble aussi présente dans les données, je ne comprends pas bien pourquoi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FlxPo tu as une idée de pourquoi ? Je ne pense pas que ça ait un énorme impact sur les résultats, mais ça m'étonne

Mind-the-Cap
Mind-the-Cap commented Feb 17, 2023
View reviewed changes
mobility/parsers/entd_2008.py

8 datasets are created and saved:

* df / short_trips: list of all short trips (<80 km as the crow flies).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Les noms des dataframes sont différents de ceux des fichiers, ça favorise la confusion. Une fois que les problèmes de fond seront éclaircis, je changerai les variables.

Mind-the-Cap
Mind-the-Cap commented Feb 17, 2023
View reviewed changes
mobility/parsers/entd_2008.py
8 datasets are created and saved:

* df / short_trips: list of all short trips (<80 km as the crow flies).
Trips with unknown length, zero length or a length > 80 km are removed.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Les trajets au-dessus de 80 km ne sont pas considérés, ni dans les trajets quotidiens ni dans les longues distances. Cela peut être un biais, en tout cas cela veut dire qu'on ne considère pas les grands mobiles.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possible qu'il y ait un biais effectivement. Il me semble qu'on avait introduit cette distinction pour ne pas avoir de double comptage avec la table voyages, mais je ne suis plus sûr de cette stratégie.

@Mind-the-Cap
Correct doc
ba3d7e1 
Améliorer la doc sur car_ownership_probability
@Mind-the-Cap
improve EMP-2019
79ae3d7
@Mind-the-Cap Mind-the-Cap changed the title Improve docstrings for ENTD-2008 parser Improve docstrings for ENTD-2008 and EMP-2019 parsers Feb 17, 2023
@Mind-the-Cap Mind-the-Cap changed the title Improve docstrings for ENTD-2008 and EMP-2019 parsers Improve docstrings Feb 22, 2023
@Mind-the-Cap
Copy link
Contributor Author

L'échantillonnage de pandas a un paramètre random_state que nous pourrions utiliser pour avoir des valeurs stables pour les tests, qu'en pensez-vous ?

@Mind-the-Cap Mind-the-Cap mentioned this pull request Feb 27, 2023
Open
2 tasks
@Mind-the-Cap Mind-the-Cap added this to the v0.1 milestone Feb 27, 2023
@Mind-the-Cap Mind-the-Cap merged commit 8364a51 into main Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FlxPo FlxPo FlxPo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Mind-the-Cap @FlxPo