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

WIP: Turn BaseField into a synonym of BaseDomain (DO NOT MERGE) #3562

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

Note that BaseField was never documented. There are two packages that reference it, though:

  1. cvec -- I already addressed that in a commit and will soon make another cvec release with that change in it (it should work with both GAP 4.10 and 4.11 that way)

  2. forms (by @jdebeule et al) -- it installs a BaseField method for IsForm objects, which however are not in IsVectorObj nor IsMatrixObj. This worked so far because we did DeclareOperation("BaseField",[IsObject]); but now it doesn't, leading to an error when loading the package. I submitted a PR to make it use InstallOtherMethod here: https://bitbucket.org/jdebeule/forms/pull-requests/1. However, that will only help once forms is released. In the meantime, we need another way to keep forms working. This is the reason why I marked his PR as "do not merge" -- I am not quite sure how to proceed best.

Of course we could just wait till there is a forms release before we apply this PR, and in the meantime, we could apply parts of it (i.e., replace any use of BaseField).

Or perhaps we remove all uses of BaseField, but still leave its BaseField declaration in, and only install a single method which delegates to BaseDomain. That declaration and the single method could go to obsolete.gd/.gi .

Or something else?

Thoughts, comments?

@fingolfin fingolfin added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes kind: discussion discussions, questions, requests for comments, and so on labels Jul 12, 2019
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I see, also the FinInG package uses BaseField.

Removing the use of BaseField from the GAP library is a good idea.

However, having BaseField as a synonym of BaseDomain is a logical problem when the result is not a field.
(For example, there is an operation FieldOfMatrixGroup that returns Rationals when applied to GL( 2, Integers ) --this result is not useful but at least correct.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: discussion discussions, questions, requests for comments, and so on release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants