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

Added a bit of clarification of how important classes attribute is to the tool doc #2231

Merged
merged 3 commits into from
Jan 24, 2023

Conversation

Tobychev
Copy link
Contributor

Besides added a minimal bit of clarification of how important classes attribute is to the tool doc, I also did some other changes to the example notebook:

  • I enabled scrolling to some of the cells so the full environment dumps don't swamp the useful content in the example
  • I added a docstring to SecondaryMyComponent, which means a later example doesn't result in a distracting error

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@maxnoe
Copy link
Member

maxnoe commented Jan 20, 2023

@Tobychev This PR contains also your benchmarks code. Probably because you started from that branch and not from master. To make sure you start a fresh branch from master for a new PR, you can do:

$ git fetch # get latest github changes
$ git switch -c <new branch name> origin/master  # start <new branch> from the current master

To fix it here, you can do git rebase -i origin/master and "drop" all commits that don't belong here and then git push -f

@maxnoe
Copy link
Member

maxnoe commented Jan 20, 2023

Ah, and also please "strip out" the notebook, e.g. Cell → All Output → Clear and Save.

@maxnoe
Copy link
Member

maxnoe commented Jan 20, 2023

The actual changes to the docs and the Tool tutorial look good!

maxnoe
maxnoe previously approved these changes Jan 20, 2023
@maxnoe
Copy link
Member

maxnoe commented Jan 20, 2023

Thanks! Looks good now.

kosack
kosack previously approved these changes Jan 24, 2023
@Tobychev Tobychev dismissed stale reviews from kosack and maxnoe via 29e5d16 January 24, 2023 13:45
@kosack kosack merged commit 2021ad1 into cta-observatory:master Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants