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

Rework tutorials #161

Merged
merged 9 commits into from
Jun 3, 2021
Merged

Rework tutorials #161

merged 9 commits into from
Jun 3, 2021

Conversation

thuiop
Copy link
Collaborator

@thuiop thuiop commented May 26, 2021

I added a tutorial covering the "advanced" features of BTK, including multiresolution, writing your own sampling function/survey/measure function ...
It is near completion but should not be merged before #153, as it will have to be updated to reflect those changes.

@thuiop thuiop self-assigned this May 26, 2021
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #161 (1ff5e2b) into main (c5fc2eb) will decrease coverage by 0.05%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #161      +/-   ##
==========================================
- Coverage   81.21%   81.16%   -0.06%     
==========================================
  Files          11       11              
  Lines        1219     1221       +2     
==========================================
+ Hits          990      991       +1     
- Misses        229      230       +1     
Flag Coverage Δ
unittests 81.16% <85.71%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
btk/sampling_functions.py 64.66% <0.00%> (ø)
btk/metrics.py 94.25% <100.00%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5fc2eb...1ff5e2b. Read the comment docs.

@thuiop thuiop requested a review from ismael-mendoza May 28, 2021 13:51
@thuiop thuiop added the Ready for review For PR which should be reviewed label May 28, 2021
Copy link
Collaborator

@ismael-mendoza ismael-mendoza left a comment

Choose a reason for hiding this comment

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

Thanks @thuiop , I'm taking a look at the notebooks separately

docs/source/tutorials.rst Outdated Show resolved Hide resolved
@review-notebook-app
Copy link

review-notebook-app bot commented May 28, 2021

View / edit / reply to this conversation on ReviewNB

ismael-mendoza commented on 2021-05-28T15:03:17Z
----------------------------------------------------------------

IMO it might be a little annoying for the users to have this many images for a tutorial notebook - maybe we can add a flag to the plotting function so that it only plots the first few galaxies?


thuiop commented on 2021-05-28T15:07:31Z
----------------------------------------------------------------

Oh, we can just slice the blended_images if needed.

ismael-mendoza commented on 2021-05-28T15:13:51Z
----------------------------------------------------------------

Ah true, could you add that? Thanks :)

@review-notebook-app
Copy link

review-notebook-app bot commented May 28, 2021

View / edit / reply to this conversation on ReviewNB

ismael-mendoza commented on 2021-05-28T15:03:18Z
----------------------------------------------------------------

Again this Shear measurement error is really annoying when batch_size is large. Is there a way to only print this error once?


thuiop commented on 2021-05-28T15:08:56Z
----------------------------------------------------------------

Actually, it is not that annoying when running it with jupyter since the cell "collapses" when it becomes too long. Otherwise we would need to catch the errors and just count them ; we can also decide to not print the error at all.

ismael-mendoza commented on 2021-05-28T15:16:21Z
----------------------------------------------------------------

oh OK I forgot about that. For the tutorial I think that it might be better to filter out the error completely and just make a note in the paragraph before the cell

ismael-mendoza commented on 2021-05-28T15:17:26Z
----------------------------------------------------------------

I think it's nice to avoid having errors printed out at least for the intro notebook (to avoid user confusion).

thuiop commented on 2021-05-31T09:33:18Z
----------------------------------------------------------------

You're right, I added a verbose parameter to the MetricsGenerator.

@review-notebook-app
Copy link

review-notebook-app bot commented May 28, 2021

View / edit / reply to this conversation on ReviewNB

ismael-mendoza commented on 2021-05-28T15:03:19Z
----------------------------------------------------------------

Line #5.                                                     target_meas={"ellipticity":btk.metrics.meas_ksb_ellipticity})

There is some weird indentation in this line


thuiop commented on 2021-05-28T15:09:27Z
----------------------------------------------------------------

Exact, I am not sure why

thuiop commented on 2021-05-31T09:35:13Z
----------------------------------------------------------------

Ok, it is just because reviewNB automatically goes to the next line while jupyter does not ; the last line is aligned with the first

Copy link
Collaborator Author

thuiop commented May 28, 2021

Oh, we can just slice the blended_images if needed.


View entire conversation on ReviewNB

Copy link
Collaborator Author

thuiop commented May 28, 2021

Actually, it is not that annoying when running it with jupyter since the cell "collapses" when it becomes too long. Otherwise we would need to catch the errors and just count them ; we can also decide to not print the error at all.


View entire conversation on ReviewNB

Copy link
Collaborator Author

thuiop commented May 28, 2021

Exact, I am not sure why


View entire conversation on ReviewNB

Copy link
Collaborator

Ah true, could you add that? Thanks :)


View entire conversation on ReviewNB

Copy link
Collaborator

oh OK I forgot about that. For the tutorial I think that it might be better to filter out the error and just make a note in the paragraph before the cell


View entire conversation on ReviewNB

Copy link
Collaborator

I think it's nice to avoid having errors printed out at least for the intro notebook (to avoid user confusion).


View entire conversation on ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented May 28, 2021

View / edit / reply to this conversation on ReviewNB

ismael-mendoza commented on 2021-05-28T15:26:06Z
----------------------------------------------------------------

Thanks this is a nice addition to demonstrate for users how to write their own sampling functions


@review-notebook-app
Copy link

review-notebook-app bot commented May 28, 2021

View / edit / reply to this conversation on ReviewNB

ismael-mendoza commented on 2021-05-28T15:26:07Z
----------------------------------------------------------------

Thanks to your helpful refactoring PR, now you can replace [survey] with survey right? Just a small thing but it might be helpful for users to see that they can do it both ways (you might also want to do this instead in the intro notebook)


@review-notebook-app
Copy link

review-notebook-app bot commented May 28, 2021

View / edit / reply to this conversation on ReviewNB

ismael-mendoza commented on 2021-05-28T15:26:08Z
----------------------------------------------------------------

Here too if you could slice the images to print less of them, that would be great


@review-notebook-app
Copy link

review-notebook-app bot commented May 28, 2021

View / edit / reply to this conversation on ReviewNB

ismael-mendoza commented on 2021-05-28T15:26:09Z
----------------------------------------------------------------

The channels_last (specifying if the channels are the first or last dimension of the image) and the survey (BTK survey object) are always passed as an argument.

I feel like this last sentence might be a bit ambiguous, since channels_last is passed explicitly as a keyword argument and survey is inside the kwargs ?


Copy link
Collaborator Author

thuiop commented May 31, 2021

You're right, I added a verbose parameter to the MetricsGenerator.


View entire conversation on ReviewNB

Copy link
Collaborator Author

thuiop commented May 31, 2021

Ok, it is just because reviewNB automatically goes to the next line while jupyter does not ; the last line is aligned with the first


View entire conversation on ReviewNB

@ismael-mendoza ismael-mendoza self-requested a review June 2, 2021 13:54
ismael-mendoza
ismael-mendoza previously approved these changes Jun 2, 2021
@ismael-mendoza
Copy link
Collaborator

I think we can merge now if you think it's ready @thuiop

@ismael-mendoza ismael-mendoza merged commit 8bb3074 into main Jun 3, 2021
@ismael-mendoza ismael-mendoza deleted the rework-tutorials branch June 3, 2021 14:12
mpaillassa pushed a commit that referenced this pull request Jul 16, 2021
* First version

* Close to complete version

* Finished advanced-tutorials and corrected average precision

* Adressed review comments

* typo on which notebook has the advanced features

* add note on how to pip install galsim_hub

* whitespace

* Added zeropoint airmass definition

Co-authored-by: Ismael Mendoza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review For PR which should be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants