search

ziotom78 / Healpix.jl

Healpix library written in Julia
GNU General Public License v2.0
51 stars 18 forks source link

rename Map #43

Closed xzackli closed 3 years ago

xzackli commented 3 years ago

This is a very questionable set of suggestions, so please feel free to close and ignore this.

  1. The name GenericMap doesn't follow the Julia convention for an abstract type, to be called AbstractSomeType.
  2. I think Map is too generic of a name, because of Julia convention to stick everything into the namespace with using. How about HealpixMap? This is mostly because I need to write a plate caree map type at some point. Similarly, SPT uses CEA.
ziotom78 commented 3 years ago

Hi @xzackli , thanks for the suggestions! Here are my thoughts:

  1. I never thought about this issue. I do not expect GenericMap to be used often in common codes, so the change GenericMap→AbstractHealpixMap is probably transparent to most of the users.
  2. That's true, and I do like HealpixMap. Perhaps we should still keep Map as a deprecated symbol and then remove it in a later release.

I would like to know the opinion of other users about these changes: @PerezHz, @hhgg, @giordano, @algebrato, what do you think about these changes?

If we all agree on these, we should decide how to fit them with the release schedule. Given that they are breaking changes, a new major release would be needed, but we are in fact on the edge of releasing version 3.0, so it would be ok to merge them and then release. Not sure about point 2, how are we supposed to manage the deprecations? Should we keep them alive until version 4.0 is out, or can we remove them in version 3.1?

ziotom78 commented 3 years ago

As there is no feedback, I assume that there are no objections with @xzackli's proposal, right? I'll create a PR as soon as I can.

ziotom78 commented 3 years ago

I have implemented the changes proposed in this issue in PR #53. I have avoided renaming readMapFromFITS to readHealpixMapFromFITS because I found it too long; moreover, it is easy to resolve any ambiguity in calling this function by explicitly stating the package:

Healpix.readMapFromFITS(…)

Please test it and report any issue.