Moved resample out of parsers to its own sub-package

[alisnwu]

closes#138

@sbillinge Ready for review

[sbillinge]

I can't merge this until it passes tests, so we need to understand why tests are not running.....

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.17%. Comparing base (0b90f3e) to head (5f285e0).

Additional details and impacted files

@@ Coverage Diff @@## main #158 +/- ##
=======================================
Coverage 99.17% 99.17% =======================================
Files 7 7 Lines 242 243 +1 =======================================
+ Hits 240 241 +1 
Misses 2 2 

Files with missing lines	Coverage Δ
tests/test_diffraction_objects.py	100.00% <100.00%> (ø)
tests/test_loaddata.py	97.77% <100.00%> (ø)
tests/test_resample.py	94.73% <100.00%> (ø)
tests/test_serialization.py	100.00% <100.00%> (ø)

[alisnwu]

@sbillinge Ready for review. PR-test successfully ran after merging Bob's codecov fix

@bobleesj I had to revert the deletions in parser's __init__.py in order to successfully import, we actually do need these

[bobleesj]

@alisnwu I will give it a quick look. @sbillinge Plz hold on for me!

[bobleesj]

@alisnwu I've tested. I recommend removing these manual relative imports below in the __init__.py file.

from .loaddataimportloadDatafrom .serializationimportdeserialize_data, serialize_data

The use of relative imports makes our documentation more confusing and we also have to maintain this file each time we create a new function.

Hence, for now you can simply change

from

from diffpy.utils.parsers import loadData

to

from diffpy.utils.parsers.loaddata import loadData

etc.

[bobleesj]

Additionally, we use lowercase_with_underscore for Python functions and typically a noun for a file name. If you could make those changes, that would be great. since diffpy.utils is used quite a bit, it would be our interest to adopt current practices.

[sbillinge]

@bobleesj is correct. We never use relative path imports and we use cobra_case. If you haven't done so yet, please take the time to read Guido van Rossum's PEP8, which we follow....but it is worth reading it.

[alisnwu]

@bobleesj I got rid of the relative path imports and renamed the functions and files, can you check if everything is good

[bobleesj]

Although this wasn't discussed before, could we try CamelCase?

https://peps.python.org/pep-0008/#class-names

[sbillinge]

Actually, not camel case which is not the PEP8 standard, but Classes always begin with capital letters. Please everyone read the PEP8 standards....

[bobleesj]

Seems it's got two interpretations:

while PEP8 says,

CapitalizedWords (or CapWords, or CamelCase – so named because of the bumpy look of its letters [4]). This is also sometimes known as StudlyCaps.

but yeah I guess CapWords works..

[bobleesj]

Actually, not camel case which is not the PEP8 standard, but Classes always begin with capital letters. Please everyone read the PEP8 standards....

yeah I will add this to our gitlab so that we can say, "did you read that section" or "refer to here.."

[bobleesj]

also - if it's camelcased, then it improves readability in lines like here

[bobleesj]

the rest looks good to me. if you also have other parts of the code that you could apply the PEP 8 standards, that would be amazing.

[bobleesj]

@alisnwu In the CI Codecov upload fails after a consecutive commit. This is indeed a bug. This wasn't happening when I was testing it. It appears it's being investigated here codecov/codecov-action#1580

I will also further look into this.

@sbillinge pytest is running well.

tests/test_tools.py 59 0 100%
tests/test_version.py 4 0 100%
-----------------------------------------------------------------
TOTAL 243 2 99%

[bobleesj]

@alisnwu I think we are using reampler.py?

[alisnwu]

Will update the news file

[bobleesj]

@sbillinge@alisnwu suddenly codecov working again! magic

[sbillinge]

@alisnwu great discussion on here. When it is resolved, let's do this over on a new clean branch, but refer back to this PR in the comments mentioning that the discussion is over here.

[Sparks29032]

@sbillinge@bobleesj removing the relative path imports in the parsers module is going to break a lot of code (such as that in diffpy.pdfgui) if you want to go through with this please make sure the imports in those packages are all updated

[bobleesj]

Thanks @Sparks29032 It seems like pdfgui won't be affected. pdfgui only uses from diffpy.utils.wx import gridutils is used. Could you confirm my understanding here?

From the package list you've mentioned in the Slack channel, it appears pdfmorph and labpdfproc will be affected due to this relative import. For diffpy.snmf and nmf, however, since they aren't publicly released, we can update these packages any time. I am not sure about clustermining

Originally we wanted to gracefully handle this using deprecation. @sbillinge will wait for your response.

[Sparks29032]

Yes, pdfgui uses its own loadData, so it is not affected. It may be good to migrate to using that of diffpy.utils so we don't have to maintain two.

The other packages have been tested, and they do not use the directory level imports (apart from PDFmorph, which has been updated in a PR). We should be good to go, but this is something we should keep in mind before we make these kinds of updates.

[bobleesj]

@Sparks29032 thanks Andrew for checking and the PRs. Indeed, I agree that we need to handle changes gracefully and it was something I figured out the last few days.