Skip to content

Bug fixes in calc_signal and get_sample#249

Merged
Didou09 merged 22 commits intodevelfrom
Issue247-bisect
Nov 17, 2019
Merged

Bug fixes in calc_signal and get_sample#249
Didou09 merged 22 commits intodevelfrom
Issue247-bisect

Conversation

@lasofivec
Copy link
Copy Markdown
Collaborator

Found a couple of bugs:

  • when sampling a line with variable resolution (los_get_sample_core_var_res): + instead of * in formula.
  • when integrating matrix in LOS_calc_signal: forgot to transpose matrix.
  • when using "minimize=hybrid" and "minimize=memory": wrong limits of LOS.

Additionally, I also took out the file _fast_sum.c in order to have less "low level" utilities and have a more maintainable code even if we lose a small percentage of optimization...

Copy link
Copy Markdown
Member

@Didou09 Didou09 left a comment

Choose a reason for hiding this comment

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

Good, but please also include a small extra unit test in tofu/tests/tests01_geom/tests03_core.py to check that the results from calc_signal() are quantitatively the same for newcalc=False and newcalc=True and for minimize = 'memory', 'call', 'hybrid' and method = 'sum', 'romb', 'simps'.
Test all combinations on a small 1d camera (10 LOS should be enough). You can re-use an already existing camera in the testing class (pick the smallest one).

=> Actually I'll deal with it right now, don't worry

@lasofivec
Copy link
Copy Markdown
Collaborator Author

I hesitated about doing this but:

  • There are already a lot of test cases doing this.
  • LOS_get_sample tests cases are the slowest
  • Adding the plot_custom_emissivity.py to the tutorials will provide a sort of unit-test

…03_Rays.test10_calc_signal() for CamLOS1D, all passing
@Didou09
Copy link
Copy Markdown
Member

Didou09 commented Nov 15, 2019

You're right,
I didn't add an extra test case, I just added an extra line (np.allclose()) to already existing cases, only for 1D cameras (100 LOS)

@Didou09
Copy link
Copy Markdown
Member

Didou09 commented Nov 15, 2019

I am also creating a new tests repository specifically to run tutorials (agree ?)

VEZINET Didier added 2 commits November 15, 2019 15:57
@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Nov 15, 2019

Hello @lasofivec! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-17 18:50:39 UTC

@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 17, 2019

Codecov Report

Merging #249 into devel will decrease coverage by 0.05%.
The diff coverage is 41.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel     #249      +/-   ##
==========================================
- Coverage   43.91%   43.85%   -0.06%     
==========================================
  Files          70       74       +4     
  Lines       21294    21462     +168     
==========================================
+ Hits         9351     9412      +61     
- Misses      11943    12050     +107
Impacted Files Coverage Δ
tofu/geom/_core.py 63.46% <ø> (ø) ⬆️
tofu/tests/tests09_tutorials/tuto_plot_basic.py 0% <0%> (ø)
...s/tests09_tutorials/tuto_plot_custom_emissivity.py 0% <0%> (ø)
...sts/tests09_tutorials/tuto_plot_create_geometry.py 0% <0%> (ø)
tofu/version.py 100% <100%> (ø) ⬆️
tofu/tests/tests01_geom/tests03_core.py 90.08% <100%> (+0.05%) ⬆️
tofu/tests/tests09_tutorials/tests01_runall.py 66.27% <66.27%> (ø)
... and 1 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 cdfb00a...ed51236. Read the comment docs.

@Didou09 Didou09 merged commit e67549f into devel Nov 17, 2019
@Didou09
Copy link
Copy Markdown
Member

Didou09 commented Nov 18, 2019

Fixes, in devel, Issue #247

@Didou09 Didou09 deleted the Issue247-bisect branch November 18, 2019 08:57
print(os.getcwd())
print(tf.__version__)
print(tf.__path__)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why these lines were added. I didn't reviewed the last version of this PR, but if you agree @Didou09 I will revert these lines.
Furthermore, the renaming of the files means that the doc construction will fail, see sphinx-gallery's doc. I'm working on making the necessary changes on another branch

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These lines were added for debugging, I forgot to remove them, yes you can delete them

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.

4 participants