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

Propagate SWITCH_RATIO to DYNAMIC_ARCH builds #3843

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

Mousius
Copy link
Contributor

@Mousius Mousius commented Nov 25, 2022

Previously dynamic builds were either using the default SWITCH_RATIO or one from the higher level architecture; this patch ensures the dynamic builds can use this parameter as well.

@Mousius Mousius force-pushed the switch-ratio branch 2 times, most recently from 0bfd59e to ed531ec Compare December 1, 2022 17:09
@Mousius
Copy link
Contributor Author

Mousius commented Dec 2, 2022

@martin-frbg can we also get this to 0.3.22? it should be a significant speed boost for those who've tuned this parameter and DYNAMIC_ARCH is used by some core data science packages (i.e. numpy, scipy)

@martin-frbg
Copy link
Collaborator

martin-frbg commented Dec 2, 2022

No fundamental objection, I'm just busy with other things. (BTW do you need the Copyright lines on everything, even where you just moved a line from one file to another ? As far as I can tell, no other contributor ever bothered with this)

@Mousius
Copy link
Contributor Author

Mousius commented Dec 5, 2022

No worries, I just noticed you added 0.3.22 to my other branch and wanted to check this one could also make that release 😸

And you're quite right on the copyright, I applied it incorrectly to all files, the move isn't valid - sorry about that!

@martin-frbg
Copy link
Collaborator

Ah don't read too much into that - I am mainly tagging PRs to make it easier to write the eventual release message and changelog, so sometimes contributions get tagged right when I merge them. So not a lot of cunning planning behind my moves

@martin-frbg
Copy link
Collaborator

@xianyi are you OK with the additional copyright notices ? This appears to be a first...

@Mousius
Copy link
Contributor Author

Mousius commented Mar 7, 2023

@martin-frbg any update on this?

@martin-frbg
Copy link
Collaborator

Very intermittent communication... would still prefer if you added yourself to CONTRIBUTORS.md like everybody else for the time being. But can easily defer this to 0.3.23, I'm struggling this week to get 0.3.22 released amid other commitments. (Next release should not take that long again or I'll lose my mind anyway...)

@Mousius
Copy link
Contributor Author

Mousius commented Mar 7, 2023

I'm not really sure what the purpose of the CONTRIBUTORS.md is? It looks like it's a subset of the git log? There only appears to be copyright claims in the various files and then in the LICENSE - so potentially there? Fear not though, I'll ask where the best place would be, was still under the impression we could use the more explicit approach despite the hostility in the linting PR.

@martin-frbg
Copy link
Collaborator

I'm not really sure what the purpose of the CONTRIBUTORS.md is? It looks like it's a subset of the git log?

git log without the granularity, I guess others may call such a thing CREDITS.txt . Anyway this seems to have served the project well enough since its inception (can't count the other 10+ years of GotoBLAS before that as that seems to have been strictly a one-man project).

There only appears to be copyright claims in the various files and then in the LICENSE - so potentially there?

Not sure if it even makes sense to add copyright claims to individual files (where they would seem to cover only a handful of lines actually affected by the change - and what to do with attributions of all previous changes, some likely pre-git ?) but I'm mostly trying to avoid opening a can of worms. Some files have "Copyright ... The OpenBLAS project", not sure if that counts for anything (I recall FLTK for one using "Copyright Bill Spitzak and others" which I guess it at least as unspecific.

Fear not though, I'll ask where the best place would be, was still under the impression we could use the more explicit approach despite the hostility in the linting PR.

My impression was that the FSFE tool was too much geared towards GPL-licensed projects, and poor at recognizing Copyright/License statements that are not verbose copies of well-known GPL/BSD/MIT headers. By not recognizing
copyright statements that are actually present if a bit non-standard and/or buried in the comments at the head of the file it seemed to create more work than it would appear to save. Rearranging our handling of the imported LAPACK tree or even the files themselves just to cater to its stubbornness certainly seemed excessive - at that point IMHO it would make more sense to go through the actual OpenBLAS files by hand IFF a suitable Copyright header was deemed necessary and agreed on.

Previously dynamic builds were either using the default SWITCH_RATIO
or one from the higher level architecture; this patch ensures the
dynamic builds can use this parameter as well.
@Mousius
Copy link
Contributor Author

Mousius commented Apr 17, 2023

@martin-frbg I think this and #3855 are now acceptable? Let me know if I can do anything more 😸

@martin-frbg martin-frbg added this to the 0.3.24 milestone Apr 19, 2023
@martin-frbg martin-frbg merged commit 437c0bf into OpenMathLib:develop Apr 19, 2023
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