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

Exposes ImageChrome option for FCS to users #503

Merged
merged 2 commits into from
Jul 5, 2021
Merged

Conversation

atruskie
Copy link
Member

@atruskie atruskie commented Jun 28, 2021

Exposes ImageChrome option for FCS to users

Now users can suppress chrome output when doing index generation of when drawing false colour spectrograms.

Changes

  • Adds a shared items project for config files so they are visible in the Visual Studio solution explorer
  • Adds ImageChrome option to config files and LDSpectrogramRGB
  • Refactors LDSpectrogramRGB so that greyscale and FCS images output at the same size, and use common global constants
  • LDSpectrogramRGB will now save a chromeless image to disk if chromeless is requested (previously it would return just the chromeless image and save the chromed image to disk)

Issues

Fixes #352

Visual Changes

Now when ImageChrome is false in an LdSpectrogramConfig, images with just raw spectral indices data will be shown.

Final Checklist

  • Assign reviewers if you have permission
  • Assign labels if you have permission
  • Link issues related to PR
  • Link any PRs or issues blocking this PR from being merged
  • Remove/Reduce warnings from edited files
  • Unit tests have been added for changes
  • Ensure CI build is passing

- Adds a shared items project for config files so they are visible in the Visual Studio solution  explorer
- Adds ImageChrome option to config files and LDSpectrogramRGB
- Refactors LDSpectrogramRGB so that greyscale and FCS  images output at the same size, and use common global constants
- LDSpectrogramRGB will now save a chromeless image to disk if chromeless is requested (previously it would return just the chromeless image and save the chromed image to disk)

Fixes #352
@atruskie atruskie requested a review from towsey June 28, 2021 04:59
@codecov
Copy link

codecov bot commented Jun 28, 2021

Codecov Report

Merging #503 (754b294) into master (7f5cfff) will increase coverage by 42.73%.
The diff coverage is 79.48%.

❗ Current head 754b294 differs from pull request most recent head 5464a20. Consider uploading reports for the commit 5464a20 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master     #503       +/-   ##
===========================================
+ Coverage    0.71%   43.45%   +42.73%     
===========================================
  Files         481      481               
  Lines       48917    48918        +1     
  Branches     7662     7667        +5     
===========================================
+ Hits          349    21255    +20906     
+ Misses      48516    27663    -20853     
+ Partials       52        0       -52     
Impacted Files Coverage Δ
...c/AnalysisPrograms/DrawLongDurationSpectrograms.cs 8.37% <0.00%> (+8.37%) ⬆️
...LongDurationSpectrograms/LDSpectrogramStitching.cs 42.18% <0.00%> (+42.18%) ⬆️
src/AnalysisPrograms/AcousticIndices.cs 66.81% <75.00%> (+66.81%) ⬆️
...Tools/LongDurationSpectrograms/LDSpectrogramRGB.cs 56.28% <83.33%> (+56.28%) ⬆️
src/AnalysisPrograms/ConcatenateIndexFiles.cs 42.82% <100.00%> (+42.82%) ⬆️
...ls/LongDurationSpectrograms/LDSpectrogramConfig.cs 76.74% <100.00%> (+76.74%) ⬆️
src/AED/EventPatternRecog.fs 7.46% <0.00%> (-86.57%) ⬇️
src/AED/SpectralPeakTrack.fs 24.13% <0.00%> (-72.42%) ⬇️
src/AED/Matlab.fs 76.19% <0.00%> (-11.12%) ⬇️
src/AED/Util.fs 22.44% <0.00%> (+1.02%) ⬆️
... and 432 more

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 7f5cfff...5464a20. Read the comment docs.

Copy link
Contributor

@towsey towsey left a comment

Choose a reason for hiding this comment

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

Concerning your PR for unchromed images I have looked at the diffs and it all looks OK to me. I notice you have previously written an extention method for booleans, i.e. the .With extention. Also I wondered why not produce both chromed and unchromed images so the user does not have another choice to think about?

@atruskie
Copy link
Member Author

atruskie commented Jul 5, 2021

I noticed the syntax ChromeImage.With and could not see if you had written an extention method for booleans

I have, but it was written when we first did chromeless images. It didn't change in this PR so you didn't see it. Good pick though.

I also decided using just a boolean would be simpler in the config file. Am open to making the enum work there too but that can be done later.

I wondered why not produce both chromed and unchromed images so the user does not have another choice to think about?

scale, performance, etc..

e.g. when running AP on the HPC cluster, I want it to output exactly what I want. Writing extra files takes a lot of time and storage space

@atruskie atruskie merged commit e26127a into master Jul 5, 2021
@atruskie atruskie deleted the fcs-no-chrome branch July 5, 2021 04:40
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.

Expose ImageChrome option in SpectrogramFalseColourConfig
2 participants