Skip to content

Conversation

alixdamman
Copy link
Collaborator

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

Copy link
Contributor

@gdementen gdementen left a comment

Choose a reason for hiding this comment

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

nice work!

# with indices (3 is out)
pop[2015, 'BruCap', X.age.i[:3], 'F', 'BE']
# with labels
year = pop.time.i[position]
Copy link
Contributor

Choose a reason for hiding this comment

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

this is unclear it uses labels. Use an explicit year instead?


- code: |
# calculates 'pop[year+2] - pop[year]'
pop.diff('time', d=2)
# warning: divnot0 may return a float array while an integer is expected
Copy link
Contributor

Choose a reason for hiding this comment

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

in this regard, it should have the same behavior than pop / divisor, right? The warning as is would make a user think this is specific to divnot0. Maybe, you should move this warning and the astype solution to the arithmetic ops section.

@alixdamman alixdamman added this to the 0.32 milestone Oct 2, 2019
Copy link
Contributor

@gdementen gdementen left a comment

Choose a reason for hiding this comment

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

I am a bit ill at ease with the change for percent.

I am unsure which of:

self * 100.0 / self.sum(*axes)
(self / self.sum(*axes)) * 100

is the more precise (my gut feeling goes for the first version), but assuming the first is not worse than the second, I would pick the first solution so that our current users model results change as little as possible (I think that way, it will only change their results if it was wrong).

In either case, please merge this PR as is or with the alternative percent change without further review.

@alixdamman alixdamman force-pushed the tutorial branch 2 times, most recently from 74db168 to 8531eb7 Compare October 7, 2019 14:43
- added generate_data.py module to generate the example and test data
- renamed 'population_session' dataset as 'demography_eurostat'
- included values for years 2016 and 2017 for all arrays of 'demography_eurostat'
- fix larray-project#785
- added the 'Pythonic VS String Syntax' section
- updated all existing sections to include changes up to the 0.31 release version
@alixdamman alixdamman merged commit 211684a into larray-project:master Oct 8, 2019
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.

2 participants