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

New package: FMM3D v1.0.1 #98072

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

JuliaRegistrator
Copy link
Contributor

@JuliaRegistrator JuliaRegistrator commented Jan 2, 2024

Copy link
Contributor

github-actions bot commented Jan 2, 2024

Your new package pull request does not meet the guidelines for auto-merging. Please make sure that you have read the General registry README and the AutoMerge guidelines. The following guidelines were not met:

  • Name does not meet all of the following: starts with an upper-case letter, ASCII alphanumerics only, not all letters are upper-case.
  • Package name similar to 1 existing package.
    1. Similar to FMM2D. Damerau-Levenshtein distance 1 is at or below cutoff of 2. Damerau-Levenshtein distance 1 between lowercased names is at or below cutoff of 1. Normalized visual distance 0.75 is at or below cutoff of 2.50.

Note that the guidelines are only required for the pull request to be merged automatically. However, it is strongly recommended to follow them, since otherwise the pull request needs to be manually reviewed and merged by a human.

After you have fixed the AutoMerge issues, simply retrigger Registrator, which will automatically update this pull request. You do not need to change the version number in your Project.toml file (unless of course the AutoMerge issue is that you skipped a version number, in which case you should change the version number).

If you do not want to fix the AutoMerge issues, please post a comment explaining why you would like this pull request to be manually merged. Then, send a message to the #pkg-registration channel in the Julia Slack to ask for help. Include a link to this pull request.

Since you are registering a new package, please make sure that you have also read the package naming guidelines: https://pkgdocs.julialang.org/v1/creating-packages/#Package-naming-guidelines


If you want to prevent this pull request from being auto-merged, simply leave a comment. If you want to post a comment without blocking auto-merging, you must include the text [noblock] in your comment. You can edit blocking comments, adding [noblock] to them in order to unblock auto-merging.

@mipals
Copy link

mipals commented Jan 4, 2024

This package is a wrapper and is therefore named after the library that it wraps.

The original library has an Apache 2.0 license. This is registration is only of the subfolder including the Julia wrapper which does not contain the license. Is this the reason why it complains regarding the license?

Regarding the name, then FMM2D is for 2D computations while FMM3D for the 3D computations. I think the differences is quite clear.

@shayandavoodii
Copy link

shayandavoodii commented Jan 4, 2024

[noblock]

Regarding the name, what if you combine the FMM2D and the FMM3D in a package called FMMs or whatever? I mean, instead of having these two packages, let's have a package that hopefully includes all the variants.

@mipals
Copy link

mipals commented Jan 4, 2024

I do not really know what is the best practice here. The two packages are wrappers to two different (but related) libraries, which would be indicated by having two separate packages. That being said I am not against having a combined package.

@lu1and10
Copy link

lu1and10 commented Jan 4, 2024

The 2D and 3D library function signatures seems to be quite similar. If @askhamwhat and @mipals both think merging the 2D and 3D is a good idea, I guess we should start a new repo and collaborate on combining the two wrappers. @askhamwhat what do you think?

@mipals
Copy link

mipals commented Jan 4, 2024

The 2D wrapper is indeed very similar (in fact it is very much just a copy-paste of the 3D wrapper with a few things changed). Note that the 2D wrapper was written quite quickly and could use a freshener at some point. In fact there are many features that it does on include as only the Helmholtz kernel was covered in the documentation at the time it was written (I do know if this has since been fixed).

Whether or not it makes sense to have a single package I do not know. Part of my likes that they are separated (due to the original packages being separate), but I can also see the idea combining them working.

Regarding the license issue then the reason is that only the subfolder is distributed. Fredrik Ekre therefore suggested on Slack to copy the license to the Julia subfolder. I guess this should be possible?

@lu1and10
Copy link

lu1and10 commented Jan 4, 2024

Regarding the license issue then the reason is that only the subfolder is distributed. Fredrik Ekre therefore suggested on Slack to copy the license to the Julia subfolder. I guess this should be possible?

Yes, @mrachh is working on editing and update the license for FMM3D, once it's done we will make a copy to the subfolder.

@maltezfaria
Copy link
Contributor

To weight in my opinion as a user of both libraries:

  • Despite the one character difference, the distinction between FMM3D vs FMM2D is quite clear
  • Since FMM2D and FMM3D wrap different libraries, I think they should exist as different Julia packages.

Thanks for trying to register this!

@tanderson92
Copy link

tanderson92 commented Jan 17, 2024

To weight in my opinion as a user of both libraries:

  • Despite the one character difference, the distinction between FMM3D vs FMM2D is quite clear
  • Since FMM2D and FMM3D wrap different libraries, I think they should exist as different Julia packages.

Thanks for trying to register this!

I am also a user of both libraries (and recently wrapped each for a project, so I'm familiar with the differences) and I would second this perspective. The difference is clear.

Moreover, wrapping them in the same way risks confusion at important implementational differences: for example, the 2D library does not (yet) support Laplace. And while the interface may be similar even for FMMLIB2D, yet another FMM library that actually does support Laplace, the FMM3D and FMMLIB2D libraries differ significantly in how they treat evaluation at source-points (targets having a nontrivial intersection with sources). Thus, in the end the user is going to need to pay heed to the documentation for the relevant library, anyway.

In fact, as the above differences show there are plenty of different FMM implementations. One can imagine kernel-independent FMMs as well sharing such a common interface. Must all these be wrapped in a single library simply because their names are somewhat similar, yet clearly and understandably distinct?

@mipals
Copy link

mipals commented Jan 17, 2024

Yeah, I am also thinking that having two separated packages is probably the way to go due to them wrapping two different libraries. No reason to mix them up.

@tanderson92 A little off topic but are you sure that the FMM2D does not support Laplace? I thought it was just undocumented (similar to Stokes and Biharmonic) and hence did not include it in the Julia wrapper.

@tanderson92
Copy link

Yeah, I am also thinking that having two separated packages is probably the way to go due to them wrapping two different libraries. No reason to mix them up.

@tanderson92 A little off topic but are you sure that the FMM2D does not support Laplace? I thought it was just undocumented (similar to Stokes and Biharmonic) and hence did not include it in the Julia wrapper.

I was speaking only to the Julia wrapper, which is still relevant for this discussion. Yes, the underlying library has more capabilities that aren't currently exposed in FMM2D.jl.

@mipals
Copy link

mipals commented Jan 17, 2024

Fair enough @tanderson92. If you at some point need the Julia wrapper of FMM2D to also include Laplace, Stokes, etc. let me know. I just stopped at Helmholtz since that was what I needed (and it was the only thing documented) and since then nobody else than me has seemed to be using the wrapper 😄

@mipals
Copy link

mipals commented Feb 28, 2024

@DilumAluthge There is a now a license in the Julia subfolder. Can the automerge be run again?

@DilumAluthge
Copy link
Member

Can the automerge be run again?

Yep, just re-trigger Registrator on your latest commit.

[noblock]

UUID: 1e13804c-f9b7-11ea-0ef0-29f3b1745df8
Repo: https://github.com/flatironinstitute/FMM3D.git
Tree: d53f013f8b1830537322729258a4dadb1d4c3a51

Registrator tree SHA: 17aec322677d9b81cdd6b9b9236b09a3f1374c6a
@JuliaRegistrator JuliaRegistrator force-pushed the registrator-fmm3d-1e13804c-v1.0.1-3923eaa56b branch from 6ed5758 to 5616505 Compare February 28, 2024 17:10
@DilumAluthge
Copy link
Member

There is a now a license in the Julia subfolder. Can the automerge be run again?

[noblock]

Looks like the license is fixed now!

@maltezfaria
Copy link
Contributor

maltezfaria commented Mar 2, 2024

[noblock]

Auto-merge fails due to the similar name conflict (FMM2D and FMM3D).

@DilumAluthge can this be manually merged now? Thanks!

@DilumAluthge
Copy link
Member

Would you be open to a longer and more descriptive name?

@maltezfaria
Copy link
Contributor

Would you be open to a longer and more descriptive name?

I think the name is descriptive enough within its niche of users (similar to e.g. FFT), but I am neither the author of the library nor the author of this PR 😅 . As a user though, I like FMM3D.jl since it corresponds to the name of the library that this package wraps.

@mipals
Copy link

mipals commented Mar 8, 2024

I agree completely with you @maltezfaria 👍

@DilumAluthge
Copy link
Member

@lu1and10 As the package author, would you be open to a longer and more descriptive name?

We usually try for package names that are descriptive to the general audience of the Julia community, not just the specific domain of users.

@lu1and10
Copy link

lu1and10 commented Mar 8, 2024

@lu1and10 As the package author, would you be open to a longer and more descriptive name?

We usually try for package names that are descriptive to the general audience of the Julia community, not just the specific domain of users.

@DilumAluthge Thanks for asking. As one of the maintainers of the FMM3D package, personally I would prefer to keep the current name FMM3D agreeing with our friends @mipals @maltezfaria . We (the main authors https://fmm3d.readthedocs.io/en/latest/ackn.html) also discussed in our internal group meeting today, we do prefer to keep the current name if it is possible. We appreciate your help.

@DilumAluthge DilumAluthge merged commit f3462db into master Mar 8, 2024
10 of 12 checks passed
@DilumAluthge DilumAluthge deleted the registrator-fmm3d-1e13804c-v1.0.1-3923eaa56b branch March 8, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants