NotificationsYou must be signed in to change notification settings
Fork15
Star22

[ISSUE #19] Make HEALPix/MOC support optional#26

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

vitcpp merged 9 commits intopostgrespro:masterfromesabol:issue-19-make-healpix-moc-optional

Jul 11, 2023

Merged

[ISSUE #19] Make HEALPix/MOC support optional#26

vitcpp merged 9 commits intopostgrespro:masterfromesabol:issue-19-make-healpix-moc-optional

Jul 11, 2023

Conversation

Copy link

Contributor

esabol commentedJul 6, 2023•
edited
Loading

This PR address issue#19 and makes support for HEALPix/MOC functionality optional. It is enabled by default, but it can be disabled by includingUSE_HEALPIX=0 as an argument to themake commands. The README.pg_sphere file is updated to reflect this change (and other recent changes to the build process).

Also, when upgrading my installation from 1.2.1 to 1.2.2, I was getting the error:

# ALTER EXTENSION pg_sphere UPDATE TO '1.2.2';ERROR:  function g_spoint3_fetch(internal, internal, internal) does not exist

I've included a fix for that in this PR (the function is justg_spoint3_fetch(internal)). Let me know if you prefer that I submit that as a separate PR.

esabol added2 commits

July 6, 2023 16:47

Make HEALPix/MOC support optional

2fe153f

Minor correction to generation of pg_sphere--1.2.0--1.2.1.sql in the …

d2fc6df

…USE_HEALPIX=1 case.

esabol changed the title~~Make HEALPix/MOC support optional~~[ISSUE #19] Added PARALLEL SAFE mark to pgSphere functions Make HEALPix/MOC support optional

Jul 6, 2023

esabol changed the title~~[ISSUE #19] Added PARALLEL SAFE mark to pgSphere functions Make HEALPix/MOC support optional~~[ISSUE #19] Make HEALPix/MOC support optional

Jul 6, 2023

Additional tweaks to README.pg_sphere

0d272eb

Copy link

ContributorAuthor

esabol commentedJul 7, 2023

make test fails, but I don't think it worked before this PR either? Does it work for you? Does anyone care?make crushtest works fine...

esabol force-pushed theissue-19-make-healpix-moc-optional branch fromceab70d to0d272ebCompare

July 7, 2023 03:55

Very minor tweak to README.pg_sphere

4eb0e3a

Copy link

Contributor

vitcpp commentedJul 7, 2023•
edited
Loading

@esabol Nice catch! You've found g_spoint3_fetch issue. Once we do not change the version number I'm ok to do it in this PR. Thank you!

Copy link

Contributor

vitcpp commentedJul 7, 2023

@esabol Concerning make test. It fails without your PR. I will create an issue for it.

vitcpp reviewed

Jul 7, 2023

View reviewed changes

Makefile Outdated

		REGRESS += healpix moc mocautocast
		endif

		REGRESS += epochprop

Copy link

Contributor

vitcppJul 7, 2023

Choose a reason for hiding this comment

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

I propose not to separate epochprop into a new line. It is already included unconditionally.

Copy link

ContributorAuthor

esabolJul 7, 2023

Choose a reason for hiding this comment

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

I didn't want to change the order of the tests, as I thought that might affect something. I think I saw a comment somewhere saying that "the order matters" and not to change it, but maybe that doesn't apply toREGRESS. If it's ok to change the order of the tests, it would certainly simplify things if I could add it unconditionally above this line and I will update the PR.

Copy link

Contributor

vitcppJul 7, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I may guess that epochprop independent on other tests. I tried to move epochprop back and forth and I haven't seen any problems with it. I got your point. It is up to you to keep PR unchanged. Furthermore, we are planning to cleanup and improve Makefile in the future. I do not see any problems with your PR at the moment.

esabol mentioned this pull request

Jul 7, 2023

make test fails#28

Closed

esabol added5 commits

July 7, 2023 16:56

Add epochprop into variable assignments for REGRESS and TESTS to simp…

26def74

…lify Makefile and also other minor cleanups to Makefile

Add USE_HEALPIX=0 cases to instrutions for regression testing and oth…

bbcbdbf

…er minor tweaks

Make Travis CI do make test and make crushtest

ec3933a

Get 'make test' working again, both with HEALPix and without

c4a106d

Add USE_HEALPIX=0 build and regression tests to Travis CI

8465435

Copy link

ContributorAuthor

esabol commentedJul 7, 2023•
edited
Loading

With the help of Travis CI, I gotmake test working again, both with HEALPix and without. Some of the line numbers were just off by one. I've included these changes in this PR since the expected output formake test is different withUSE_HEALPIX=0 and without, so I think it's related. This work addresses issue#28 as a result. I think we should keepinit_test.sql. I think it's meant to be used to test the buildbefore you install it. So perhaps the README should recommend that the procedure should bemake; make test; make install; make installcheck; make crushtest?

I've also addedmake test andmake crushtest to the Travis CI and added a separateUSE_HEALPIX=0 build and it's testing. All this adds approximately 1 minute to each CI job or about 6 minutes for all builds in PR. I mention this since I know Travis CI costs money. These changes address issue#27.

That just leaves the minimum PostgreSQL version mentioned in theREADME.pg_sphere file. I've changed it to version 9.6 in the most recent commits. Other than that, this PR is ready to merge if you like what you see. Thanks!