Wrote notebook on synthetic Mosviz image creation #636
Conversation
ojustino
requested review from
duytnguyendtn,
ibusko,
javerbukh and
rosteen
as code owners
Codecov Report
@@ Coverage Diff @@
## main #636 +/- ##
==========================================
+ Coverage 59.67% 59.69% +0.02%
==========================================
Files 65 65
Lines 4000 4332 +332
==========================================
+ Hits 2387 2586 +199
- Misses 1613 1746 +133
Continue to review full report at Codecov.
|
|
As per your comment, I converted this PR to draft status. You can convert it back to "ready" when it is ready. FYI. |
ojustino
changed the title
|
codecov fails but this this a notebook so it may not apply |
|
FYI: For the future, please open a PR from a branch on your fork, not on this repo. |
There was a problem hiding this comment.
Very nice and detailed walkthrough. Thanks!
Branding question: Is it "Mosviz" or "MOSViz" in the narrative doc? Maybe @javerbukh knows. But then again, maybe this is not important to hash out within the "concepts" notebooks.
Disclaimer: I did not run the cells that has %%script and require VPN.
Minor comments below.
Also, the paths are not Windows-friendly but you don't have to do anything about it here; just something I noticed.
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "%%script false --no-raise-error\n", |
There was a problem hiding this comment.
Instead of inserting %%script, why not convert the cell to "raw NBconvert" type instead? That way, when someone wants to run it, they convert it back to actual code cell.
There was a problem hiding this comment.
Are there issues with %%script to be concerned about? I was hoping to preserve those cells' syntax highlighting while also not having them run by default.
There was a problem hiding this comment.
It is fine if you want to keep them, especially since this is an exploratory notebook.
| "source": [ | ||
| "fig, ax = plt.subplots(figsize=(10,10))\n", | ||
| "ax.imshow(synth_image, vmin=0, vmax=synth_image.std(), **imshow_params)\n", | ||
| "#ax.imshow(synth_image, vmin=0, vmax=synth_image.mean()*3, **imshow_params)\n", |
There was a problem hiding this comment.
| "#ax.imshow(synth_image, vmin=0, vmax=synth_image.mean()*3, **imshow_params)\n", |
|
@pllim Thanks for the review. My mistake on opening a pull request from upstream -- I actually already have my fork and remote set up and can open a new PR once the notebook is cleared for merging. I could work on putting the data on Box in order to drop the VPN requirement. I addressed most of your comments in the new commit and left a reply on the last one. |
I've opened this PR to get this notebook that I wrote with @robelgeda on the record, but please consider it a draft until we hear more from other stakeholders in the JWST data pipeline discussion. I realize the notebook still contains some paths that are specific to my laptop and that the style may not be fully compliant with
jdavizstandards.