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

First PR! added reltol and abstol to rank(A) in linalg/generic.jl #19014

Closed
wants to merge 8 commits into from
Closed

First PR! added reltol and abstol to rank(A) in linalg/generic.jl #19014

wants to merge 8 commits into from

Conversation

miguelraz
Copy link
Contributor

Just tried adding some basic keyword functionality to the funciton.

@StefanKarpinski
Copy link
Sponsor Member

This breaks tests and compatibility since the old form rank(M, tol) doesn't work. In general, when you submit a PR, you should try running the tests (at least the relevant ones) locally first. In this case, that means make test-linalg since the rank function appears in various test/linalg files.

It's also probably easier to get some buy-in for and API change like this before submitting a PR.

@miguelraz
Copy link
Contributor Author

@StefanKarpinski Thanks a lot for the feedback. I have just noticed something with my notifications and will try to be more attentive to the thread.

I didn't run tests because I just assumed that if merge with master was compatible, all was good.

Will run tests and adjust accordingly.

If you have any other recommendations that are worth hearing, I'm all for it.

See ya around, and thanks!

~~@StefanKarpinski~~ Julia needs an underling to give this PR a shot. [#13942](#13942).
1. I know I'm clearly missing some important things, but I hope the effort counts. I couldn't find a way to get the rows from the array comprehension...
2. Willing to start over from scratch - any comments/critiques/suggestions more than welcome.
3. I am first trying a MWE with simple structure and inputs. Then I will try to get something that works more generically. Is this a good way to procceed?

[ci skip]

Roadmap:
* Verify each returned array is the same type as the types of the elements
* Verify lengths of all arrays to be unzipped
* Return as a single Array of Arrays or as separate Arrays?
* Remove that `zip is its own inverse` in `@doc zip`?
* Revise documentation phrasing and example
~~@StefanKarpinski~~ Julia needs an underling to give this PR a shot. [#13942](#13942).
1. I know I'm clearly missing some important things, but I hope the effort counts. I couldn't find a way to get the rows from the array comprehension...
2. Willing to start over from scratch - any comments/critiques/suggestions more than welcome.
3. I am first trying a MWE with simple structure and inputs. Then I will try to get something that works more generically. Is this a good way to procceed?

[ci skip]

Roadmap:
* Verify each returned array is the same type as the types of the elements
* Verify lengths of all arrays to be unzipped
* Return as a single Array of Arrays or as separate Arrays?
* Remove that `zip is its own inverse` in `@doc zip`?
* Revise documentation phrasing and example
@miguelraz miguelraz closed this Mar 29, 2017
@miguelraz
Copy link
Contributor Author

miguelraz commented Mar 29, 2017

This PR got too mucky. Will attempt again with a new one.
Not another classic episode of Github panic

@miguelraz miguelraz deleted the rank_tolerance_fix branch March 29, 2017 01:30
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.

None yet

2 participants