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

RFC: consider @v macro for value indexing #118

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

Conversation

ajkeller34
Copy link
Collaborator

@ajkeller34 ajkeller34 commented Sep 11, 2017

Ref. #84, #117.

I don't intend/expect to merge this, mainly I put it up here as food for thought. I support using value indexing by default, but whatever the implementation I think there ought to be a first-class way to use normal indexing if desired (I would not be okay with having to do A.data[1,2] to use normal indices, for example). It would also be nice to provide some mechanism to adjust the tolerances when doing floating-point comparisons, though I feel less strongly about that since the default tolerances usually work fine.

This approach does not switch to value indexing by default, but provides (at least in my view) a convenient alternative that satisfies my concerns:

  • Easy syntax
  • Easy to splice in normal indexes where desired for mixed indexing
  • Pretty obvious how to specify custom tolerances
  • No breaking changes (which is nice, though it's not one of my main concerns)

It has some downsides:

  • This is probably backwards. Indexes should be interpreted as values by default, and then perhaps we could have a macro like this to use normal/mixed indexing? Maybe there's a better way.
  • No awareness of whether an array being indexed is actually an AxisArray, so one has to be a little careful. There's no way around this with a macro, apart from implementing generic fallback methods for how an AbstractArray should handle Value objects as indices, which seems clumsy.
  • @v is pretty terse for an exported symbol, but @value would be super annoying to type

@mlubin
Copy link

mlubin commented Sep 12, 2017

This makes sense but doesn't address my use case; see comment at #117 (comment).

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