-
Notifications
You must be signed in to change notification settings - Fork 11
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
Use new MultiplyColumnwiseBandMatrixField alias #2229
base: main
Are you sure you want to change the base?
Conversation
This also caused me a lot of troubles and hard-to-debug problems (see #1771). However, I think that the difference between ⋆ and * is way too small for ⋆ to be an acceptable symbol to use: it's extremely hard to distinguish and will cause a lot of confusion EDIT: I misread the PR. Using ⋆ is not the intended interface, so the problem won't be as big. That said, if this is internal, I would still lean towards a different symbol. |
💯
That's fair. I really don't have a preference on what we use, especially since this is temporary. I went through the effort of replacing all of the places where we need to use a new symbol, while skipping places where we need to use LinearAlgebra's How about help?> ◌
"◌" can be typed by \dottedcircle<tab> |
Sounds good with me! |
◌ is similar to Base's function composition operator, and it isn't generally an appropriate symbol for multiplication. I think it would be much simpler and clearer to use the standard multiplication symbol * instead, so I'll open a PR for that. |
Which operator is that? |
697786c
to
d720850
Compare
I was referring to the following operator:
I really don't see a need to export any new symbol (either ⋆ or ◌), when we could just use the standard *. Adding more symbols that are hard to distinguish is not the direction we want to go in, even if they are temporary. I have updated #2232 to retain the value of ⋅ exported in this PR, while also allowing us to switch ClimaAtmos/ClimaLand/ClimaCoupler to *, so let's use that PR instead. |
The
\cdot
alias forMultiplyColumnwiseBandMatrixField
is a pain-- it collides with LinearAlgebra's\cdot
, and it's exported byClimaCore.MatrixFields
.This PR doesn't fix this issue per-say, but I would like to at least isolate where we are doing this and fix collisions for the ClimaCore examples.
I think we unfortunately can't remove the export from
ClimaCore.MatrixFields
, as this will technically be a breaking change. Perhaps we can deprecate this change and phase it out.I spoke with @dennisYatunin, and he believes that we can replace all of these
\cdot
s with*
. This PR applies these changes to all of the appropriate places, but instead replacing it with\star
. This will allow us to do a replace-all once we are ready to apply this fix.