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

Explicitly declare type of index arrays #8

Closed
wants to merge 1 commit into from

Conversation

eschnett
Copy link

This closes issue #7 .

Type of pull request
Grammar fixes don't require any further explanation.
Documentation extensions or examples require justification.
Bug fixes require inclusion of or reference to bug report.
New features require justification and desired goals.

Describe the feature
A clear and concise description of what the feature is.
Describe the changes proposed in the pull request.
Reference any related issue in repository.

To Reproduce
Steps to reproduce the desired behavior:

Expected behavior
A clear and concise description of what you expected to happen.

Environment: (if necessary)

  • OS: [e.g. GNU/Linux/OSX/Windows]
  • Version [e.g. Julia v1.4, Grassmann v0.5.7]

Additional context
Add any other context about the problem here.

@chakravala
Copy link
Owner

The type should not be explicitly declared, since it isn't always going to be Int. If you check with @code_warntype the method is already type stable, and sometimes it will not return an Int. For example, one might define the metric with Float64 instead of Int.

@chakravala chakravala closed this Apr 25, 2020
@chakravala
Copy link
Owner

chakravala commented Apr 25, 2020

Actually, I'll leave this PR open for now, but the provided solution is not correct.

I think what you need is a special case handling for when the array is empty.

The current solution will not be sufficient, since I do not wish to restrict to Int types.

@chakravala chakravala reopened this Apr 25, 2020
@chakravala chakravala added the enhancement New feature or request label Apr 25, 2020
@chakravala chakravala force-pushed the master branch 2 times, most recently from e61dc06 to 1a12109 Compare July 1, 2020 02:22
@chakravala chakravala closed this May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants