Skip to content

Conversation

Spinachboul
Copy link

@benfulcher @joshuabmoore @fkiraly
Solving issue #72

This initial implementation will be the starter modified base class of pyspi, and an attempt to make a unified public API. Keeping it draft until I finalize commits
These will be the scope of the upcoming commits:

  • Refactor the base classes, modularize the classes BaseSPI, Directed and Undirected.
  • Implement specific logic for spi and spi_mat functions.
  • Standardize the decorators ( to be discussed further )

Your reviews and guidance would be highly appreciated in this PR Thanks a lot!!

pyspi/base.py Outdated
def _spi(self, data: Data, i: int = 0) -> float:
raise NotImplementedError("Subclass must implement _spi.")

@parse_univariate
Copy link

Choose a reason for hiding this comment

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

I would not use these as decorators at all! But move the logic as a call into spi.

@Spinachboul Spinachboul requested a review from fkiraly May 2, 2025 19:50
@Spinachboul
Copy link
Author

@joshuabmoore Does the current API design for the class align well.
I plan to move the logic as a call to SPI as suggested by @fkiraly.
Ideally a better way to handle this would be to incorporate the tagging system to handle different utilities. I have drafted that in the code for the subclasses for better demonstration and understanding.

@Spinachboul
Copy link
Author

@fkiraly
Is the API design suitable for the base class, then I shall proceed with the full function implementations
Thanks!!

Copy link

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

I would recommend you leave the current base classes untouched to ensure downwards compatibility. Add the new base class and try to write three substantially different spi as a descendant, include example usage in the concrete class docstring.

Copy link

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

  • the original base class is still changed - can you revert that for now?
  • the new base class looks mostly reasonable - I would suggest to move ahead and try implement two concrete SPI (perhaps slightly different ones).
  • also, try to add tests for the SPI. For now, it is fine to loop over the concrete classes (once you have them)

@Spinachboul Spinachboul marked this pull request as ready for review May 21, 2025 16:39
Copy link

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Great! This fixes the point with reverting changes to the old base class.

Not addressed yet:

  • try implement two concrete SPI (perhaps slightly different ones).
  • try to add tests for the SPI. For now, it is fine to loop over the concrete classes (once you have them)

@Spinachboul
Copy link
Author

@fkiraly @benfulcher @joshuabmoore
I have added a new SPI named TransferEntropy
Also I have made some changes in the API
so the the current SPI would be accessed as pyspi.spi.transfer_entropy
Would like to know if this design works
And if not, how should the new SPIs be added for the new base class

Thanks!!

1 similar comment
@Spinachboul
Copy link
Author

@fkiraly @benfulcher @joshuabmoore
I have added a new SPI named TransferEntropy
Also I have made some changes in the API
so the the current SPI would be accessed as pyspi.spi.transfer_entropy
Would like to know if this design works
And if not, how should the new SPIs be added for the new base class

Thanks!!

@Spinachboul
Copy link
Author

@fkiraly @joshuabmoore @benfulcher
I have added a new SPI called Transfer Entropy
This SPI would be called as pyspi.spi.transfer_entropy`
Would like to know if this API design works.
And if not, how should the SPIs be added for the new base class

Thanks!!

Copy link

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Looks nice!

I would recommend next:

  • add tests
  • add at least two SPI which are already in the code base

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