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

Graphics for file flow #684

Merged
merged 12 commits into from
Dec 18, 2020
Merged

Graphics for file flow #684

merged 12 commits into from
Dec 18, 2020

Conversation

lea-hagen
Copy link
Member

This adds visualizations of what files are created when a BEAST run is split by either subgrids or source density. These are added to the docs and the code is in plotting. To help untangle the mess of lines, some of them are color-coded to separately follow the SED grid, noise model, and photometry.

plot_graphic_file_flow.py has options to change the numbers of subgrids, SD bins/sub-bins, etc. I settled on the current numbers because sometimes the graphs get rearranged in unhelpful ways.

Partially addresses #680


Subgrid splitting
beast-file-flow-subgrid


Source Density splitting
beast-file-flow-sourceden

@lea-hagen
Copy link
Member Author

I'll check on the codestyle things. But in the meantime, @karllark do you happen know what's going on with docs error about not being able to read the image file?

@lgtm-com
Copy link

lgtm-com bot commented Dec 17, 2020

This pull request introduces 1 alert when merging a5629c3 into b1950c6 - view on LGTM.com

new alerts:

  • 1 for Unused import

@karllark
Copy link
Member

@karllark do you happen know what's going on with docs error about not being able to read the image file?

You need to add the png file to the [options.package_data] section in setup.cfg. Png files are not automatically included (unlike py files).

@lea-hagen
Copy link
Member Author

Oh right! I'm pretty sure you've had to explain that to me a few times now 😆

@lea-hagen
Copy link
Member Author

Hmm, that seems to not be the problem. I think that line in setup.cfg is for things in the beast folder, not in the docs. Looking back at #624, there weren't changes to any configuration files (except for adding the graphviz dependency), and as far as I can tell, those images worked fine.

@karllark
Copy link
Member

Humm....do the docs build on your computer? Another thought, are the permissions on those files ok?

@karllark
Copy link
Member

karllark commented Dec 17, 2020

Ok, the issue is the filename is wrong in the rst file. In the docs it is images/beast-graphic-file-flow-sd.png and the file is actually called images/beast-graphic-file-flow-sourceden.png.

@lea-hagen
Copy link
Member Author

Thanks for catching that!! Hopefully things are good to go now...

@lea-hagen
Copy link
Member Author

It looks like the coverage test is failing because it's having download problems. I noticed that yesterday too, but with a different file.

@karllark
Copy link
Member

I agree the coverage tests are an unrelated download issue.

Can you fix the minor codacy issue?

@lea-hagen
Copy link
Member Author

I just checked on the Codacy issue. I'm a little confused - it says "No blank lines allowed after function docstring", but only finds one instance of it, when there are actually three functions with what I assume are blank lines after the function docstring. Is this something you're familiar with?

@karllark
Copy link
Member

Is this something you're familiar with?

Nope. If black is ok with the file, then say the word and I'll ignore the codacy check.

@lea-hagen
Copy link
Member Author

lea-hagen commented Dec 18, 2020

Yeah, I re-blackened and nothing was changed. I did some googling, and from discussion here, it seems like it might be varying interpretations of one of the PEPs:
https://github.com/PyCQA/pydocstyle/issues/361

(and apparently I inadvertently linked that issue and this PR... woops 😬)

@karllark
Copy link
Member

One last minor change. Can you change the title of the rst page to Graphical Models as now there is more than one?

@lea-hagen
Copy link
Member Author

Done!

@codecov-io
Copy link

Codecov Report

Merging #684 (93ce6db) into master (b1950c6) will decrease coverage by 0.67%.
The diff coverage is 4.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #684      +/-   ##
==========================================
- Coverage   44.91%   44.24%   -0.68%     
==========================================
  Files          98       99       +1     
  Lines        9477     9638     +161     
==========================================
+ Hits         4257     4264       +7     
- Misses       5220     5374     +154     
Impacted Files Coverage Δ
beast/main.py 16.12% <ø> (ø)
beast/plotting/plot_graphic_model.py 24.00% <ø> (ø)
beast/plotting/plot_graphic_file_flow.py 4.43% <4.43%> (ø)
beast/tools/density_map.py 21.21% <0.00%> (-0.67%) ⬇️
beast/tools/run/make_ast_inputs.py 14.75% <0.00%> (ø)
beast/tools/split_catalog_using_map.py 14.54% <0.00%> (ø)
beast/observationmodel/ast/make_ast_xy_list.py 5.19% <0.00%> (ø)

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 b1950c6...93ce6db. Read the comment docs.

Copy link
Member

@karllark karllark left a comment

Choose a reason for hiding this comment

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

Looks great.

@karllark karllark merged commit 44d12e3 into BEAST-Fitting:master Dec 18, 2020
@lea-hagen lea-hagen deleted the file_flow branch December 18, 2020 23:14
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.

3 participants