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

Expose getting orbital params from name #251

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pfebrer
Copy link
Contributor

@pfebrer pfebrer commented Aug 8, 2020

I exposed the convertion from orbital name to n, l, m, Z, P as an static method. This can be very convenient for the visualization module (and maybe other cases) and does not disturb the functionality of AtomicOrbital (it passes all tests and does not add extra computation). It also adds some possibilities like providing part of the parameters as a string and the rest as kwargs:

from sisl import AtomicOrbital

AtomicOrbital("3p", m=0, P=True)
AtomicOrbital("3", l=2, m=1, Z=2)
AtomicOrbital("3dZ2", m=1)

Also, I think there was some 3-fold repeated code regarding the setup of spherical orbital, so I condensed it all in one go. It passes all tests so I guess it is correct.

Can this go in so that the visualization module can act magically? :)

@codecov
Copy link

codecov bot commented Aug 8, 2020

Codecov Report

Merging #251 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #251   +/-   ##
=======================================
  Coverage   85.10%   85.11%           
=======================================
  Files         131      131           
  Lines       20818    20817    -1     
=======================================
+ Hits        17717    17718    +1     
+ Misses       3101     3099    -2     
Impacted Files Coverage Δ
sisl/orbital.py 94.93% <100.00%> (+0.51%) ⬆️

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 c4f6457...fef3904. Read the comment docs.

@zerothi
Copy link
Owner

zerothi commented Aug 10, 2020

Thanks for this.

What exactly is it you want? 👍
I.e. I am a bit reluctant to do this since one can easily do:

aorb = AtomicOrbital(name)
aorb.n
aorb.l
aorb.m
aorb.Z
aorb.P

If what you want is to select orbitals based on criteria, wouldn't it make sense to extend the AtomCategory parts to do this?

@zerothi
Copy link
Owner

zerothi commented Aug 10, 2020

Additionally there are some problems with this logic, you will force a passed name to superseede explicit arguments: "5px", n=2 will force n=5, while I would argue that explicit arguments have precedence.

But don't do anything yet, I think we should first clear out what you want to achieve.

@pfebrer
Copy link
Contributor Author

pfebrer commented Aug 10, 2020

What exactly is it you want?

Basically I have information stored for each orbital in a DataArray (e.g. the PDOS or the weights for the fatbands). The "orbital" index is just an integer specifying the index of each orbital, because using quantum numbers as indexers would make the dataarray sparse and therefore memory inefficient and harder to query (this is what happens when you do pdos_sile.read_data(as_dataarray=True).

Then, I have a pandas Dataframe where the information is stored for each orbital. I.e. it looks something like this:

n l m Z name
0 . . . . .
1 . . . . .
2 . . . . .

I use the n, l, m, Z and name inputs to select an orbital and thanks to pandas this is very simple. With this, I get the indices of the orbitals that I use to query the data from the dataarray.

Then, the things that I want:

  • The "name" parameter does not make sense of course if I can convert from a name to the numbers beforehand. Additionally this would save memory. This can be solved by doing AtomicOrbital(name).
  • I want to be able to accept incomplete names such as "2p" so that the user can easily query all "2p" orbitals. This was not possible with the current implementation because doing AtomicOrbital(name) assumes that the name is complete. Therefore it tries to guess some parameters (n) and raises an error if it can not determine others (e.g. m). I want to know the fully determined parameters and get None for those that can not be determined from the name.

All in all, I want an orbital selector basically, so implementing it with categories sisl-wide can be nice. Also it will probably be better than my implementation, which currently stores the information as many times as the orbital is present in the geometry.

More generally, I think it would be cool to have a class to store orbital resolved data that you can query easily (e.g. mydata.sel(orbital="2s") or mydata.sel(l=3)). Basically I think of it as a wrapper for a DataArray that would use orbital indices to store the data and would parse orbital queries to convert them to indices (in my approach this is done by the helper DataFrame, but it would be more efficient with categories). Another approach would be to build multi indexed DataFrames, but in my opinion this is more complex and still you would need to parse the queries to support querying with incomplete names, so you would need to wrap it too if you want to automatize it.

Additionally there are some problems with this logic, you will force a passed name to superseede explicit arguments: "5px", n=2 will force n=5, while I would argue that explicit arguments have precedence.

Doesn't this happen already with the current implementation? Anyway, it could easily be solved.

@zerothi
Copy link
Owner

zerothi commented Sep 16, 2020

I like this idea of sel, or at least a variant of it.

One thing that really requires some careful thought is how it should be implemented. Here are some thoughts:

  • pandas indexing and location methods (sel, loc, iloc) returns a new dataframe,
  • xarray similar behaviour as pandas

I see a couple of possibilities here.

  • add Geometry.index which basically returns indices corresponding to the indexers, I am imagining an interface like def index(self, indexers, base='orbitals'|'atoms'|None), where the default base is determined from indexers, say if they are all orbital specifications, then it is clear that base is orbitals.
  • this also calls for some kind of wrapper for data that has a geometry associated, this goes in line with Create an attribute class which can accept events #196 (I think)
  • should data always be ndarray subclasses with additional sisl features? This isn't totally clear to me. I think this opens up a can of worms... I have actually very rarely seen proper and good extensions of ndarray, but perhaps I am not knowledgeable about successfull projects doing this?

Any ideas would be very nice!
`

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