Skip to content

Fixes #gh-82 Draft of Black Hole case study content #160

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

Merged
merged 11 commits into from
Feb 15, 2020

Conversation

shaloo
Copy link
Contributor

@shaloo shaloo commented Jan 26, 2020

Black Hole Case Study draft added

Deployed URL

http://numpy-<pr_number>.surge.sh/

@shaloo shaloo requested a review from rgommers January 26, 2020 16:20
@shaloo
Copy link
Contributor Author

shaloo commented Jan 26, 2020

Here is how the draft looks using basic Hugo layout available -

Screenshot 2020-01-26 at 9 48 27 PM

Screenshot 2020-01-26 at 9 48 35 PM

Screenshot 2020-01-26 at 9 48 57 PM

Screenshot 2020-01-26 at 9 49 09 PM

Screenshot 2020-01-26 at 9 49 17 PM

@shaloo
Copy link
Contributor Author

shaloo commented Jan 26, 2020

@joelachance, do we have any hugo theme shortcodes to help align and size images better? Also, are there any shortcakes for quote text - similar to how medium allows it? Say for e.g., https://github.com/parsiya/Hugo-Shortcodes

@shaloo
Copy link
Contributor Author

shaloo commented Jan 28, 2020

@rgommers do you have any feedback for this case study? Is it good for merging?

After noticing BIDS-numpy/numpy-paper#51 I am also wondering how best to utilise this work into our showcase (for gravitational waves as well as M87 imaging) Any ideas?

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Visually this looks pretty good, based on the screenshot. I can't get it to work locally though.

The text reads quite well to me. One thing to be careful about is that you name Katie/Andrew without emphasizing much that they were two people out of a much larger team. E.g., eht-imaging has a number of contributors with a large number of commits, not just Andrew. Could you try to tweak that a little?

@rgommers
Copy link
Member

Sorry for the delay in reviewing @shaloo. The overall style, length and use of imagery is really nice I think - this is what we need from a case study I think.

@rgommers
Copy link
Member

rgommers commented Feb 1, 2020

After noticing BIDS-numpy/numpy-paper#51 I am also wondering how best to utilise this work into our showcase (for gravitational waves as well as M87 imaging) Any ideas?

The gravitational waves part has been dropped again from that paper, the black hole part will stay in. I'd say feel free to take over any content and adapt the wording to make it fit in the case study. The info does give some more context into how NumPy and other libraries were used, so is useful.

@shaloo shaloo requested a review from rgommers February 2, 2020 15:02
@shaloo
Copy link
Contributor Author

shaloo commented Feb 2, 2020

@rgommers - updated as suggested. Thx.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Text shows up well now, but can't see figures

@shaloo
Copy link
Contributor Author

shaloo commented Feb 8, 2020

This is how the page renders with Hugo on Mac OSX (catalina). I've tried to attribute credits to original image/diagram with links. Hope this addresses @rgommers inputs. thanks.

Screenshot 2020-02-09 at 12 16 53 AM

Screenshot 2020-02-09 at 12 17 02 AM

Screenshot 2020-02-09 at 12 17 10 AM

Screenshot 2020-02-09 at 12 17 18 AM

![Uploading Screenshot 2020-02-09 at 12.17.33 AM.png…]()

@shaloo shaloo closed this Feb 8, 2020
@shaloo shaloo reopened this Feb 8, 2020
@shaloo shaloo requested a review from rgommers February 8, 2020 18:50
@rgommers
Copy link
Member

Thanks for the fixes and screenshot @shaloo. It now works correctly for me.

I switched machines but I think it was the actual changes here and not Hugo version.

rgommers
rgommers previously approved these changes Feb 10, 2020
@rgommers
Copy link
Member

rgommers commented Feb 10, 2020

We'll want to remove the "Case Studies" link again when the tiles on the front page are merged I think, but for now it's useful:

image

In it goes! Thanks @shaloo, this is great to have:)

EDIT: oh no, hold on - there's a second case study in this same PR now!

@rgommers rgommers dismissed their stale review February 10, 2020 04:20

missed a part, re-reviewing

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

The second case study looks pretty good! My one important comment is about the visualization part.

@shaloo
Copy link
Contributor Author

shaloo commented Feb 10, 2020

We'll want to remove the "Case Studies" link again when the tiles on the front page are merged I think, but for now it's useful:

image

In it goes! Thanks @shaloo, this is great to have:)

EDIT: oh no, hold on - there's a second case study in this same PR now!

Yes, I struggled with #163 and then realised until BH one is merged GW can't go in - then thought of merging both to simplify first draft. Will refine in subsequent PRs when I add more CS.

@shaloo shaloo requested a review from rgommers February 10, 2020 05:13
@shaloo shaloo requested a review from Shekharrajak February 13, 2020 17:30
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This is looking pretty good now! All images look nice and are captioned with credits.

There's some room for polishing the text and probably would be useful if we'd ping some content matter experts for review of the astro/physics parts. But that can be done after merging, I'd like to get this in.

@shaloo the one thing I would like to see before merging is optimizing the included images for size. There's one of 2.2 MB and several more in the 0.5-1 MB range. It should be possible to compress them by a factor of a couple without losing resolution I'd think. Could you try that?

@shaloo
Copy link
Contributor Author

shaloo commented Feb 15, 2020

This is looking pretty good now! All images look nice and are captioned with credits.

There's some room for polishing the text and probably would be useful if we'd ping some content matter experts for review of the astro/physics parts. But that can be done after merging, I'd like to get this in.

@shaloo the one thing I would like to see before merging is optimizing the included images for size. There's one of 2.2 MB and several more in the 0.5-1 MB range. It should be possible to compress them by a factor of a couple without losing resolution I'd think. Could you try that?

Done.

@rgommers rgommers merged commit b01153b into numpy:master Feb 15, 2020
@rgommers
Copy link
Member

Awesome, in it goes. Thanks @shaloo!

@shaloo shaloo deleted the issu_82 branch March 2, 2020 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants