-
Notifications
You must be signed in to change notification settings - Fork 69
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
Cleanup hom functions for FinGenAbGrp
and TorQuadModule
#1835
base: master
Are you sure you want to change the base?
Conversation
This should not be merged, yet. I need to fix the deprecations. I just wanted to open the discussion first, see if there were any strong opinions against this change. If not, then I will go through Hecke to fix the deprecations. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1835 +/- ##
==========================================
- Coverage 76.61% 76.47% -0.14%
==========================================
Files 362 362
Lines 115012 115035 +23
==========================================
- Hits 88119 87977 -142
- Misses 26893 27058 +165
🚀 New features to boost your workflow:
|
Let me add that the |
I am happy to always have |
No strong opinion from Kaiserslautern. |
src/GrpAb/Map.jl
Outdated
function hom( | ||
G::FinGenAbGroup, | ||
H::FinGenAbGroup, | ||
A::Vector{FinGenAbGroupElem}, | ||
B::Vector{FinGenAbGroupElem}; | ||
check::Bool=true | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we want the types of A
and B
as stated. The method will complain if there is some type instable or sloppy code and we get a Vector{Any}
. These errors can be confusing for beginners.
Do we care? Maybe it would be better to just put A::Vector
? Any opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Just put Vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
Needed some cleanup, and there were some problems for trivial groups/empty lists of images.
Deprecate some previous methods which relied on empty lists in input which were not checked; add preferable methods where we enforce an input parent.
@simonbrandhorst