Tutorial: miscellaneous #914

alixdamman · 2021-02-17T14:54:59Z

slight changes or improvements here and there

review-notebook-app · 2021-02-17T14:55:03Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

alixdamman · 2021-02-17T15:47:25Z

From the logs from Travis, I can see that the newer version of pandas has broken LArray...

gdementen

tutorial: use print() everywhere: -1 on this. We can add a note about this/explain this at the start if this isn't the case already but users need to understand that rather sooner than later because most Python ressources on the net use that syntax.
tutorial: use string syntax to generate axes in the getting_started: +0
tutorial: removed the warning box about how to define Session objects…: +1
tutorial -> tutorial_presenting_larray_objects notebook:
- use the string syntax to declare axes: +0 in general but a -1 for the "age axis with strings" change as is. I think it is a good idea to show the consequence of having a mixed axis (need to index with strings) but it should contain some kind of explanation that it is a special case when you have a mixed axis. Without an explanation , this is going to confuse more than anything IMO. Also, I wonder if this is the best place to introduce that.
- removed other axis (it is unlikely that users will mix letters and numbers when generating labels with the syntax start..end): +0
- do not use numpy randint() function to generate fake data (to avoid to confuse readers): +1
- avoid to define anonymous axes when calling function to create arrays with predefined values (confusing): +1
- removed example explaining how to create Session objects with Python 3.5- (versions 3.5 or less are too old): +1 but see inline comment.
tutorial -> tutorial_combine_arrays notebook:
- removed subsection about extend() (redundant with append()/append()/insert()): strong -1 as this is really not the same operation. We should probably make it so (document that .append also works when the axis already exists + deprecate .extend #887) but it is not the case yet, and even if it was documented, keeping an example where the axis exist is a must IMO.
- added example using argument after instead of before with the insert() method: +1

gdementen · 2021-02-17T15:35:20Z

doc/source/tutorial/tutorial_presenting_larray_objects.ipyml

    # create an anonymous Group object 'teens'
-    teens = age[10:20]
+    teens = age['10':'18']


Beurk! See general comment.

gdementen · 2021-02-17T16:08:54Z

doc/source/tutorial/tutorial_presenting_larray_objects.ipyml

@@ -364,12 +369,6 @@ cells:
    gender = Axis("gender=Male,Female")
    time = Axis("time=2013..2017")

-    # create and populate a new session in one step
-    # Python <= 3.5
-    demography_session = Session([('gender', gender), ('time', time), ('population', zeros((gender, time))), 


Note that this syntax is also useful when you have objects with names which are not valid Python identifiers (such as labels starting with a number). This probably needs to be mentioned somewhere, but probably not here.

Using a dictionary instead of a list of pairs would be clearer. Anyway, it is difficult and maybe not a good idea to show any pitfall or very specific cases in the tutorial. IMO, the tutorial is big enough right now to not add (very) specific cases (at least this one).

Indeed a dict is better for Python3.7+, my bad. My comment was only valid for 3.6, but since we are doing away with it too, you can indeed simply remove that. Concerning special/tricky cases I think a tutorial is meant precisely to show those which users are likely to stumble into and how to "work around" them. In this case, I don't know how likely it is. I have seen array labels starting with numbers quite a few time, but never in a session, so removing the block is fine I guess.

Your comment above makes me think that I may add something about how to create a group mixing separated labels and a slice (using Group.union()). What do you think ?

A good idea (unless you plan to tackle #429 "soonish", in which case it would be better to tackle it first to show the nice syntax directly) but I don't know what's the best place to add that...

alixdamman · 2021-02-18T08:18:41Z

* tutorial: use print() everywhere: -1 on this. We can add a note about this/explain this at the start if this isn't the case already but users need to understand that rather sooner than later because most Python ressources on the net use that syntax.

OK. OK. I did that because 2 of our users send me an email to complain about the fact that lines like:

population.info

don't print anything when they run their scripts. They think that everything in the tutorial is transferable to their scripts as it is.
But you're right when you say "users need to understand that rather sooner than later because most Python ressources on the net use that syntax". I will reverse that commit and add a note somewhere.

* tutorial -> tutorial_presenting_larray_objects notebook:
  
  * use the string syntax to declare axes:  +0 in general but a **-1** for the "age axis with strings" change as is. I think it is a good idea to show the consequence of having a mixed axis (need to index with strings) **but** it should contain some kind of explanation that it is a special case when you have a mixed axis. Without an explanation , this is going to confuse more than anything IMO. Also, I wonder if this is the best place to introduce that.

OK

* tutorial -> tutorial_combine_arrays notebook:
  
  * removed subsection about extend() (redundant with append()/append()/insert()): strong **-1** as this is really not the same operation. We should probably make it so (#887) but it is not the case yet, and even if it was documented, keeping an example where the axis exist is a must IMO.

Hum. Well. What does really extend() is obscure to me and I can't find an example where extend() cannot be replaced by either insert() or append() or prepend(). To tell the truth, I never use it when I have to help someone.

alixdamman · 2021-02-18T10:18:30Z

Do you have an example in mind where the use of extend() cannot be replaced by either insert(), prepend() or append() ?

gdementen · 2021-02-19T10:05:17Z

doc/source/tutorial/getting_started.ipyml

@@ -27,6 +27,17 @@ cells:
    __version__


+- markdown: |
+    <div class="alert alert-warning">
+        The tutorial is generated from Jupyter notebooks which works in "interactive" mode (like in the LArray Editor). 


s/works/work

LArray Editor console?

gdementen · 2021-02-19T10:06:36Z

doc/source/tutorial/getting_started.ipyml

+- markdown: |
+    <div class="alert alert-warning">
+        The tutorial is generated from Jupyter notebooks which works in "interactive" mode (like in the LArray Editor). 
+        In the "interactive" mode, there is no need to use the function print() to display the content of a variable.


s/the "interactive"/interactive/
s/the function print()/the print() function
also add a mention of expressions?

gdementen · 2021-02-19T10:09:28Z

doc/source/tutorial/getting_started.ipyml

+        The tutorial is generated from Jupyter notebooks which works in "interactive" mode (like in the LArray Editor). 
+        In the "interactive" mode, there is no need to use the function print() to display the content of a variable.
+        Simply writing its name is enough.
+        The function print() is only need if the content of more than one variable has to be displayed.<br><br>


I would remove this sentence (as it is not entirely true), you can display a tuple in the console:

>>> a = 1; b = 2 >>> a, b (1, 2)

gdementen · 2021-02-19T10:12:44Z

doc/source/tutorial/getting_started.ipyml

+        Simply writing its name is enough.
+        The function print() is only need if the content of more than one variable has to be displayed.<br><br>
+        In a Python script (file with .py extension), you always need to use the print() function to display the content of 
+        a variable or the value returned by a function.


add a mention of expressions?

gdementen · 2021-02-19T10:28:44Z

OK. OK. I did that because 2 of our users send me an email to complain about the fact that lines like:
population.info
don't print anything when they run their scripts. They think that everything in the tutorial is transferable to their scripts as it is.

I know, I already had that question too.

gdementen · 2021-02-19T10:34:35Z

Concerning the print() function, I think it would help if you mentioned

an explicit example. Something like: "For example, we did not need to use a print() function to display the content of the __version__ variable above".
explicit explanation that when copy pasting code from the tutorial in a script file, you need to add those print() calls.

gdementen · 2021-02-19T10:49:01Z

Do you have an example in mind where the use of extend() cannot be replaced by either insert(), prepend() or append() ?

Probably not something user-friendly, because .append does work in that case (by accident -- this is the whole point of #887) but that's not the point here. The point is that those two methods were meant to do (and are documented as doing) different things and even if we can do both "things" using the same method, we should show both in the tutorial. So either you leave it as is (using .extend) or you tackle #887 first/at the same time and change that .extend call to .append.

gdementen · 2021-02-19T10:51:16Z

PS: the difference between larray.append and .extend was meant to be exactly the same as the difference between list.append and .extend

gdementen · 2021-02-24T11:05:38Z

doc/source/tutorial/tutorial_presenting_larray_objects.ipyml

+
+
+- code: |
+    # mixing a slices and individual labels leads to the creation of a tuple of groups 


s/a slices/slices/
s/a tuple of/several distinct/ ?

gdementen · 2021-02-24T11:05:57Z

doc/source/tutorial/tutorial_presenting_larray_objects.ipyml

+- markdown: |
+    <div class="alert alert-warning">
+
+    **Warning:** Mixing slices and individual labels inside the `[ ]` will generate a **tuple of groups** instead of unique group.<br>If you want to create a unique group using both slices and individual labels, you need to use the `.union()` method (see below).   


s/a tuple of/several distinct/ ?
s/of unique/of a single/
s/a unique/a single/

gdementen · 2021-02-24T11:11:52Z

doc/source/tutorial/getting_started.ipyml

    </div>


+- code: |
+    s = "There is no need to use the print() function to display the content of the variable 's'."


couldn't having the message within the variable be confusing?

gdementen · 2021-02-24T11:14:36Z

doc/source/tutorial/getting_started.ipyml

+
+
+- code: |
+    "There is no need to use the print() function " + "to display the returned value of an " + "expression"


Use something simpler?

>>> 1 + 2 3

gdementen · 2021-02-24T11:16:23Z

doc/source/tutorial/tutorial_aggregations.ipyml

+- markdown: |
+    <div class="alert alert-warning">
+
+    **Warning:** Mixing slices and individual labels inside the `[ ]` will generate a **tuple of groups** instead of unique group.<br>If you want to create a unique group using both slices and individual labels, you need to use the `.union()` method (see below).   


s/a tuple of/several distinct/ ?
s/of unique/of a single/
s/a unique/a single/

gdementen · 2021-02-24T11:16:55Z

doc/source/tutorial/tutorial_aggregations.ipyml

+
+
+- code: |
+    # leading to unexpected results


to potentially unexpected?

gdementen · 2021-02-24T11:17:09Z

doc/source/tutorial/tutorial_aggregations.ipyml

+
+
+- code: |
+    # the union() method allows to mix slices and individual labels to create a unique group


gdementen · 2021-02-25T15:06:46Z

LGTM, it's merging season 😉

gdementen · 2021-02-25T15:09:29Z

btw the "the" you added were not typos. I think it's correct too but it's superfluous. In general, "the" is used less in English than "le" in French.

…setup.py

- use string syntax to generate axes - removed the warning box about how to define Session objects before Python 3.6 (Python version previous to 3.6 are too old now) - removed other axis (it is unlikely that users will mix letters and numbers when generating labels with the syntax start..end) - do not use numpy randint() function to generate fake data (to avoid to confuse readers) - avoid to define anonymous axes when calling function to create arrays with predefined values (confusing) - removed example explaining how to create Session objects with Python 3.5- (versions 3.5 or less are too old) - added example using argument after instead of before with the insert() method - added a warning box to explain why we don't use the print() function to display the content of a variable in the tutorial - added warning and examples about how LArray infer type of labels - added explanation about how to create a unique group using slices and individual labels together

alixdamman requested a review from gdementen February 17, 2021 14:54

gdementen reviewed Feb 17, 2021

View reviewed changes

gdementen reviewed Feb 19, 2021

View reviewed changes

gdementen reviewed Feb 24, 2021

View reviewed changes

alixdamman added 2 commits February 25, 2021 16:21

miscellaneous: removed pytest-pep8 from the required dependencies in …

408df41

…setup.py

alixdamman force-pushed the tutorial branch from 41ab4be to 82222ed Compare February 25, 2021 15:21

alixdamman merged commit 0f02f12 into larray-project:master Feb 25, 2021



		- code: \|
		# mixing a slices and individual labels leads to the creation of a tuple of groups



		- code: \|
		"There is no need to use the print() function " + "to display the returned value of an " + "expression"



		- code: \|
		# the union() method allows to mix slices and individual labels to create a unique group

Tutorial: miscellaneous #914

Tutorial: miscellaneous #914

Uh oh!

Conversation

alixdamman commented Feb 17, 2021

Uh oh!

review-notebook-app bot commented Feb 17, 2021

Uh oh!

alixdamman commented Feb 17, 2021

Uh oh!

gdementen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alixdamman commented Feb 18, 2021

Uh oh!

alixdamman commented Feb 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdementen commented Feb 19, 2021

Uh oh!

gdementen commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gdementen commented Feb 19, 2021

Uh oh!

gdementen commented Feb 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdementen commented Feb 25, 2021

Uh oh!

gdementen commented Feb 25, 2021

Uh oh!

Uh oh!

gdementen commented Feb 19, 2021 •

edited

Loading