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

Add support for iterating EXtra-data SourceData objects #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

philsmt
Copy link
Collaborator

@philsmt philsmt commented Mar 22, 2022

While extra_data.SourceData does not have native support for iterating yet, I figured it's actually fairly easy to do on the pasha side. This has the upsides that it will continue to work for older versions even once added there.

@takluyver While not really part of the public API, would you consider the initializer of DataCollection private?

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #14 (c4cec68) into master (b9e7564) will decrease coverage by 2.26%.
The diff coverage is 0.00%.

❗ Current head c4cec68 differs from pull request most recent head 769f086. Consider uploading reports for the commit 769f086 to get more accurate results

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
- Coverage   91.34%   89.07%   -2.27%     
==========================================
  Files           5        5              
  Lines         358      366       +8     
==========================================
- Hits          327      326       -1     
- Misses         31       40       +9     
Impacted Files Coverage Δ
pasha/functor.py 68.04% <0.00%> (-7.24%) ⬇️

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 b9e7564...769f086. Read the comment docs.

@takluyver
Copy link
Member

While not really part of the public API, would you consider the initializer of DataCollection private?

🤔 I wouldn't encourage people to depend on it, especially passing lots of parameters. I try to avoid breaking even private things if practical, but e.g. I changed the meaning of the second parameter in 1.10 (European-XFEL/EXtra-data#274) in a way that would probably break something like this. And I can imagine that the 'origins' idea we talked about earlier today might involve further changes.

@philsmt
Copy link
Collaborator Author

philsmt commented Mar 22, 2022

e.g. I changed the meaning of the second parameter in 1.10 [...] in a way that would probably break something like this.

Indeed, I thought I remember the initializer differently 😉

Ultimately I would like to move the ExtraDataFunctor into EXtra-data proper, so it can freely use internal APIs. I just happened to want to iterate over sources several times in the past weeks. So maybe that's the best course of action?

@takluyver
Copy link
Member

Can we design it so that pasha can recognise EXtra-data objects without either having to import the other? E.g. IPython looks for a _repr_html_() method (among others) to display objects in the notebook - we could look for something like a _pasha_functor_() method.

@philsmt
Copy link
Collaborator Author

philsmt commented Mar 22, 2022

Of course, that was the intent. pasha supports this via a _pasha_functor_ method that you actually inspired 😉

@takluyver
Copy link
Member

I see, so it needs to return something with .split() and .iterate() methods. That should be doable.

If .split() is expected to return a list of slices for plain integer indexing, I wonder if we can simplify this a bit, so the returned object can define __len__() rather than .split(), and pasha will call gen_split_slices() using the len() it gets.

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