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

Clean up namespace #49

Merged
merged 10 commits into from
Feb 22, 2021
Merged

Clean up namespace #49

merged 10 commits into from
Feb 22, 2021

Conversation

jlaehne
Copy link
Contributor

@jlaehne jlaehne commented Jan 27, 2021

Based on the comment from at #17 (comment) I cleaned up the namespace in analogy to pyxem/pyxem#704.

Progress:

  • Changes implemented
  • Adapted tests
  • Check again
  • Ready for review

Example:
Signals would be initiated as in HyperSpy and pyxem:
s = lum.signals.LumiSpectrum(np.arange(10))

@jlaehne
Copy link
Contributor Author

jlaehne commented Jan 27, 2021

I'm thinking of joining all the three CL related files in signals into cl_spectrum.py. I do not think we need to have so many different files, what do you think @jordiferrero @dnjohnstone . I've seen that in pyxem quite a number of different (though related) signal types are in one file.

@jordiferrero
Copy link
Contributor

I'm thinking of joining all the three CL related files in signals into cl_spectrum.py. I do not think we need to have so many different files, what do you think @jordiferrero @dnjohnstone . I've seen that in pyxem quite a number of different (though related) signal types are in one file.

I am not sure about this...
When we discussed the initial signals structure, we realised that we needed some CL signals as some functions are only relevant for SEM and others are more general. I personally only work with SEM_CL data and like to have a special file with the functions for it (it's easier to navigate through classes).

I assume you are proposing to have the 3 classes in one file, right?
Happy to discuss further, but I would not merge them.

@jlaehne
Copy link
Contributor Author

jlaehne commented Feb 4, 2021

Yep, that is exactly what I was thinking. 3 classes in one file. So we would have files for EL, PL, CL, but subclasses would be in the same.

I don't think there will be so many functions that are specific only to SEM or TEM. So far I mostly still have my signals as CLSpectrum.

We could also merge them now and if it ever gets too crowded still separate them.

Just some ideas, I am not passionate about either solution.

@jordiferrero
Copy link
Contributor

@jlaehne I agree to go ahead and merge the 3 classes in one file.
If you update this PR with those changes we can aprove it.

@jlaehne
Copy link
Contributor Author

jlaehne commented Feb 22, 2021

done

(I tried to introduce a test for a warning from #44, but that would require the github-actions to include a hyperspy-gui package, so it is commented out for the moment. We can add that in a later PR to improve coverage.)

@jordiferrero jordiferrero merged commit 5a5f6a1 into LumiSpy:master Feb 22, 2021
@jlaehne jlaehne deleted the namespace branch February 22, 2021 19:43
@jlaehne jlaehne mentioned this pull request Feb 22, 2021
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