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.

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

Chapter 15 -- internal review #786

Closed
Nowosad opened this issue May 2, 2022 · 14 comments
Closed

Chapter 15 -- internal review #786

Nowosad opened this issue May 2, 2022 · 14 comments
Assignees

Comments

@Nowosad
Copy link
Member

Nowosad commented May 2, 2022

Hi @jannes-m -- is the Ecology chapter ready for our internal review?

@jannes-m
Copy link
Collaborator

jannes-m commented May 3, 2022

Yes, it is 😊

@Nowosad Nowosad self-assigned this May 3, 2022
@Nowosad
Copy link
Member Author

Nowosad commented May 7, 2022

Hi @jannes-m,

I read the whole chapter, made some minor changes (#793), and also have some questions/comments for you:

  1. There were a few changes in {vegan} that influenced calculations in this chapter (https://cran.r-project.org/web/packages/vegan/news.html). Look at the 15-eco-12 code chunk. Currently, this code does not run 500 iterations, but only 20.
  2. Another related topic -- see the xy-nmds-code code chunk. The scores() function is now a list with two elements, and thus the code in line 303 does not work.
  3. This also influences the 15-eco-16 code chunk.
  4. "Overall, we can interpret the tree as follows: the higher the elevation, the higher the NMDS\index{NMDS} score becomes." Can you elaborate on this statement? What does it mean in practical terms? What can we learn from the results?
  5. Line 471 -- maybe it would be good to cite the {ranger} package there?
  6. Lines 488-490. How did you decide on these limits of hyperparameters?
  7. The https://mlr-org.github.io/interpretable-machine-learning-iml-and-mlr/ link is broken.

@Nowosad Nowosad assigned jannes-m and unassigned Nowosad May 7, 2022
@Nowosad
Copy link
Member Author

Nowosad commented May 7, 2022

Also -- please recheck the exercises in this chapter -- I cannot reproduce some of them.

@jannes-m
Copy link
Collaborator

Hey @Nowosad,
thanks a lot for the review. Will have a look at it as soon as things settle down a little bit here with three kids. Sorry for being a bit unresponsive the last couple of weeks. In any case I appreciate your comments and will incorporate them. The same goes for the comments you have raised regarding the statistical learning chapter. Please note also that I have already updated the code of the Geomarketing chapter and as soon as I can I will also update the prose. However, I can't really tell when this will be but don't worry I will do it -:)

@Nowosad
Copy link
Member Author

Nowosad commented May 18, 2022

Hi @jannes-m -- no problem. Family is always a priority.

@Robinlovelace
Copy link
Collaborator

Hi @jannes-m are you able to work on this?

My current thinking is that the issues raised by @Nowosad above are sufficiently minor that they can be fixed during or after the review process. I'm looking at the issues related to submitting the 3rd part for peer review and this seems the biggest 'blocker', that does not actually need to be a blocker.

https://github.com/Robinlovelace/geocompr/milestone/10

@jannes-m
Copy link
Collaborator

jannes-m commented Oct 9, 2022

Will work on this the coming week.

@jannes-m
Copy link
Collaborator

There were a few changes in {vegan} that influenced calculations in this chapter (https://cran.r-project.org/web/packages/vegan/news.html). Look at the 15-eco-12 code chunk. Currently, this code does not run 500 iterations, but only 20.

This is indeed the case, but we are only saying the algorithm should try for 500 times if necessary to reach a solution. If it can come up with an optimal solution without having to use 500 trials, all the better. It also says so in the text.

@jannes-m
Copy link
Collaborator

jannes-m commented Oct 10, 2022

Another related topic -- see the xy-nmds-code code chunk. The scores() function is now a list with two elements, and thus the code in line 303 does not work.
This also influences the 15-eco-16 code chunk.

Good catch, thanks a ton! It would be awesome if it also solved the 15-eco-16 code chunk problem! Taken care of via #ee19a40c

"Overall, we can interpret the tree as follows: the higher the elevation, the higher the NMDS\index{NMDS} score becomes." Can you elaborate on this statement? What does it mean in practical terms? What can we learn from the results?

I have added following two sentences:
This means that the simple decision tree has already revealed four distinct floristic assemblages.
For a more in-depth interpretation please refer to the @ref(predictive-mapping) section.

Line 471 -- maybe it would be good to cite the {ranger} package there?

Changed as requested.

Lines 488-490. How did you decide on these limits of hyperparameters?

I have added the corresponding reference: [@probst_hyperparameters_2018]

The https://mlr-org.github.io/interpretable-machine-learning-iml-and-mlr/ link is broken.

I have replaced it with the new link.

@jannes-m
Copy link
Collaborator

One of the problems was in fact that scores() now returns a list, therefore, the rp object could not be created. I really have no idea how I could miss that, therefore, thanks a lot again @Nowosad for pointing this out!

@Nowosad
Copy link
Member Author

Nowosad commented Oct 11, 2022

Hi @jannes-m one small suggestion; can you add small comments to some packages used in this chapter, e.g., library(paradox) #do this and that? It could be helpful for the readers. (See https://geocompr.robinlovelace.net/attr.html)

@jannes-m
Copy link
Collaborator

Just fyi @Nowosad,I have added the short package descriptions.

@Nowosad
Copy link
Member Author

Nowosad commented Oct 12, 2022

Hi @jannes-m where? I cannot find it in the book... Is it in some branch?

@jannes-m
Copy link
Collaborator

branch ecology_finetuning

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

No branches or pull requests

3 participants