Skip to content

Conversation

AndrewKillion
Copy link

@AndrewKillion AndrewKillion commented Sep 26, 2025

Description

  • Add abstract transform_coords method to Transform base class
  • Implement method in all concrete Transform classes
  • SimplexTransform and SumTo1 remove last coordinate
  • Most other transforms preserve coordinates
  • Add comprehensive test suite

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pymc--7913.org.readthedocs.build/en/7913/

…rdinate. All others added identity function.

Fixes pymc-devs#7907
Copy link

welcome bot commented Sep 26, 2025

Thank You Banner]
💖 Thanks for opening this pull request! 💖 The PyMC community really appreciates your time and effort to contribute to the project. Please make sure you have read our Contributing Guidelines and filled in our pull request template to the best of your ability.

@jessegrabowski
Copy link
Member

Thanks for opening this!

Docs are failing because you missed adding transform_coords to a few transformers. My suggestion would actually be to use multiple inheritance in this case, we can have a OneToOneTransformerMixin class that just defines transform_coords(coords): return coords, and you can use that everywhere.

@ricardoV94
Copy link
Member

OneToOneTransformerMixin class that just defines transform_coords(coords): return coords, and you can use that everywhere.

Have a subclass of Transform that behaves like that, and use that? Why mixin?

@ricardoV94
Copy link
Member

Actually all these with ndim_supp=0, are those cases. Scalar transforms that don't modify coords

@jessegrabowski
Copy link
Member

Have a subclass of Transform that behaves like that, and use that? Why mixin?

Mixins are cool and hip. sklearn does mixins!

Anyway a subclass would accomplish the same thing as well, and I have no strong preference for one over the other.

@ricardoV94
Copy link
Member

I just don't see what it accomplishes, seems more complex than it needs to be

@jessegrabowski
Copy link
Member

I guess it's meant to give classes a more "modular" feel? It's not that a transfomer that doesn't mutate coords isn't a transformer (it is), it just also a coordinate transformation behavior defined by a OneToOneCoordMixin. A OneToOneTransfomer subclass would accomplish the same thing, but it would feel conceptually somewhat more removed from the Transformer base class, which it isn't.

@ricardoV94
Copy link
Member

ricardoV94 commented Oct 12, 2025

Let's do it when we have a reason for needing that modularity. These are not really things we're composing or intending to in the foreseeable future

@jessegrabowski
Copy link
Member

Like I said, I don't object to a subclass. I'm just making the case for a mixin.

@ricardoV94
Copy link
Member

I object to a mixin and I'm not making a case but I can

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.

3 participants