Skip to content

test/func: created a test and function for MorphFuncy #186

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 2 commits into from
Apr 24, 2025

Conversation

Luiskitsu
Copy link
Contributor

I have created a draft for the MorphFuncy class and tested it using different functions with various parameters that I have defined using pytest.parametrize. I am currently defining the parameters for the functions as a dictionary, where each function has its specific parameters. I'm unsure if using a dictionary is the best decision. We can implement more options such as if the user does not provide a function to raise a warning or something similar.

Copy link

codecov bot commented Apr 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.11%. Comparing base (4914488) to head (b2c46cb).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #186      +/-   ##
==========================================
+ Coverage   96.99%   97.11%   +0.12%     
==========================================
  Files          19       20       +1     
  Lines         831      866      +35     
==========================================
+ Hits          806      841      +35     
  Misses         25       25              
Files with missing lines Coverage Δ
tests/test_morphfuncy.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

wow, fantastic. I few tiny cosmetic comments, but you nailed it in one shot as far as I can tell!

import pytest

from diffpy.morph.morphs.morphfuncy import MorphFuncy

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these functions would be better with single blank lines between them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am trying to delete these but every time I commit the files are modified by the hooks. I think this is the pre-commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK,, we can let it slide. It is because normally we want the functions well separated, but wehn we are defining a bunch of them for testing we just want them kind of closed up, but it is not a big deal.

Just fix the docstring and I can merge this.

morph = MorphFuncy()
morph.function = function
morph.parameters = parameters
x_morph_actual, y_morph_actual, x_target_actual, y_target_actual = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you have the function call inside parens? It should work the same way without these and this is somewhat unconventional and therefore distracting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is same as comment above, this is modified by the hooks.

-------
Import the funcy morph function:
>>> from diffpy.morph.morphs.morphfuncy import MorphFuncy
Define or import the user-supplied transformation function:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this would be easier to read with judiciously placed blank lines, maybe after each code block, but play around till it looks nice? I am not sure if there is a docstring standard for this, in which case follow it, but if not, then I think a few blank lines could increase readability.

@Luiskitsu
Copy link
Contributor Author

Thanks Simon! This is your mentoring ... after MorphSqueeze I did learn a lot.
As I mentioned in the comments, a lot of the cosmetic issues are modified by the hooks. Not sure how I can change these.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

pls see inline

import pytest

from diffpy.morph.morphs.morphfuncy import MorphFuncy

Copy link
Contributor

Choose a reason for hiding this comment

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

OK,, we can let it slide. It is because normally we want the functions well separated, but wehn we are defining a bunch of them for testing we just want them kind of closed up, but it is not a big deal.

Just fix the docstring and I can merge this.

@sbillinge sbillinge merged commit a450cd7 into diffpy:main Apr 24, 2025
5 checks passed
@Luiskitsu
Copy link
Contributor Author

@sbillinge should I work on MorphFuncx? Do we want to do the same as MorphFuncy but applied to the x-axis and then return the morphed data in the same grid as the input morph data? It would also be good if I could see the code for PDFgetX3, I can't find it in github.

@sbillinge
Copy link
Contributor

@sbillinge should I work on MorphFuncx? Do we want to do the same as MorphFuncy but applied to the x-axis and then return the morphed data in the same grid as the input morph data? It would also be good if I could see the code for PDFgetX3, I can't find it in github.

I do want this functionality in the fullness of time, and it should be relatively straightforward because we have the squeeze morph that uses a "particular" function which was the polynomial. So it should be relatively quick, almost a copy-paste.

However, I would rather push next to see if we can figure out how to use a getX3 function to do the moprh onto the synchrtron data......

@Luiskitsu
Copy link
Contributor Author

@sbillinge should I work on MorphFuncx? Do we want to do the same as MorphFuncy but applied to the x-axis and then return the morphed data in the same grid as the input morph data? It would also be good if I could see the code for PDFgetX3, I can't find it in github.

I do want this functionality in the fullness of time, and it should be relatively straightforward because we have the squeeze morph that uses a "particular" function which was the polynomial. So it should be relatively quick, almost a copy-paste.

However, I would rather push next to see if we can figure out how to use a getX3 function to do the moprh onto the synchrtron data......

Sounds great to me!
Where can I find the documentation for getX3 functions?

@sbillinge
Copy link
Contributor

let me go and dig that up. This code is not open source.....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants