rlancaste / stellarsolver

GNU General Public License v3.0
89 stars 47 forks source link

Is it necessary to use the included versions of sep and astrometry.net? #84

Closed 8Keep closed 2 years ago

8Keep commented 2 years ago

Fedora has a (soft?) requirement that packages don't bundle other packages when they could be using a shared lib instead. This has caused stellarsolver packaging to get blocked, and kstars is very outdated because of it.

https://bugzilla.redhat.com/show_bug.cgi?id=1938451

I want to know if there is something special about these included libs or if it is fine to use the already packaged and up to date versions.

rlancaste commented 2 years ago

Hi there, are you referring to the included sep and astrometry folders inside that we use to build StellarSolver? If so, then yes they are absolutely required. The files inside those folders are not the original sep and astrometry.net libraries or files. They are very much based on the original sep and astrometry packages that are publicly available, but they are definitely not the same and we could not use the actual original libraries to achieve the same purpose. It took me over a year (with the help of of others as well) to make all of the changes to those files in those folders that are required to make this cross platform solver work. If you would like to see what changes were made, I noted every change I had to make with an easily searchable comment noting that the change was made and why. There are literally hundreds of changes.

For astrometry, I did show all the changes to the astrometry.net authors, and we have their blessing, but they are not willing to make all those changes to astrometry.net at this time. A great many of the changes were required to make the software run natively on windows, but the astrometry team decided that is not a priority for them. More changes were needed for us to internally get access to some variables and set parameters internally in the libraries without using external config files. Also note that I only changed and kept files we needed for the system to work for us, to actually port the whole astrometry project to windows would take a lot more time.

For SEP, Jasem made the changes to that one not me, but those changes were critical as well. SEP does not natively support parallel programming or threads due to some global variables. He made changes that would allow us to do it, which are critical for our program to work efficiently.

if we used the publicly available sep and astrometry libraries, the solver would not work on windows natively at all, it would require external config files, would read and write temporary files frequently to the system (bad if your os is on a raspberry pi), and the multi treading functionality that makes it so powerful would not work.

Does this answer the question or were you referring to something else?

8Keep commented 2 years ago

Thanks so much for the context! Sounds good, I think we'll be able to package it all in one and not rely on the existing libraries.

rlancaste commented 2 years ago

Very good. Yeah the context is very important. I can understand that if you are working on a Linux system only, that shared libraries is very much the way to go, and if different projects depend on other software, it would be silly to include all dependencies in the project, especially if they would get outdated. But in this project, we really could not do that and the bulk of the work was actually in modifying those libraries to get them to work with us and each other. So yes I know it seems to go against the way things are usually done, but I appreciate that an exception can be made for this project. If you need anything else please let me know.

8Keep commented 2 years ago

I actually have nothing to do with the fedora project or redhat, I'm just interested in getting this package into fedora so kstars can be updated to a recent release :)

Somebody did try to get it packaged, but ran into this blocker (see my link). I'll give them a few days, if nobody's available, I'll try to submit the new package review and hopefully then it'll be an easy task to update kstars.

rlancaste commented 2 years ago

Sounds good to me. Of course getting an up to date kstars on all platforms would be my highest priority too.

pemensik commented 2 years ago

I have made diff between latest astrometry.net and files in stellarsolver. Indeed, there is bunch of fixes. Most of changes I have seen seem to be Windows specific compilation fixes. Bunch of memory leak issues, which seems should be included in original astrometry.net project. @rlancaste have you tried offering at least subset of your changes back to original project? Would you please try opening pull requests to some issues found?

I understand there are differences. But after quick glance, modified code does not seem functionally much different. It seems to add mostly ability to compile under MS Visual Studio. I think any useful changes should be proposed to original project. Even if that change is not sufficient to compile whole astrometry.net under windows, you could still use only partial changes.

For example, every change we do in distribution software should be proposed to upstream maintainers. That way software gets more and more similar instead more and more different.

I think there is far more changes to sep library. Except for some constants and get_filenames function, I saw very little functional changes on compilation for Linux. It would be much better, if they could be toggled by some set_stellarsolver_mode(true); library call, when most of the code is just exactly the same. Could required functionality be merged into astrometry.net? It seems possible to me.

rlancaste commented 2 years ago

So for the memory leak issues, I don't think they were as concerned about that because of the way astrometry.net is used. It is meant to be a command line program that runs once and exits. One of my goals was to put it in a library so that it could be used without having to call an external executable. In the library form, we immediately ran into the memory leak issues since each time we used the code, the leaks started adding up, since the program is still running. That isn't to say they wouldn't appreciate knowing about my memory leak fixes, since I did actually mention that to him/them when I explained what I had done, and he seemed surprised that I had found memory leaks, but he was open to receiving fixes to that. I definitely think this would be good to do, but I haven't had time to do it yet.

The goal was to keep the functionality mostly the same, so yes, it should work as similarly as possible to the original astrometry.net. I wasn't setting out to change astrometry.net, just make it cross platform, work as a library, and not use all the external configuration files. The biggest difference, as you identified, would be that I worked very hard to get it to work/build natively under windows. In fact, when I first started stellarsolver, I was just using the shared astrometry.net dynamic libraries on linux and Mac, but it was when I tried to make it work on windows that that approach no longer worked. I did have to make a lot of changes to make it work on Windows. The astrometry.net folks seemed a lot less interested in those changes/fixes. And as I noted above, the windows fixes that I made only affected a very small part of the code. To actually get it all to work on windows would be a huge task. The problem is that the StellarSolver library is really meant to be cross platform and astrometry.net is definitely not cross platform in its current form. They just aren't interested in supporting windows right now (as of the last year when I shared this project and what I had done with them).

I do think that merging fixes upstream is a very good goal, but I don't have time to make a bunch of changes to stellarsolver right now, especially if it is going to cause an issue with building and releasing KStars or cause an interruption or a bunch of bugs to show up. I am a full time high school teacher and I have a lot more time in the summer than I do during the school year. I was able to make all that progress on StellarSolver in the spring of 2020 due to the onset of the pandemic and the fact that we were basically shut down for a few weeks before switching to virtual learning. I made a lot more progress in the summer when I was off work. I had mostly perfected StellarSolver by the fall and then we went to integrate it back into KStars, and it was working great, but then after we did that, there were suddenly a bunch of proposed changes to it from various people and those changes broke things for a couple of weeks as we worked out the kinks. I just don't want something like that to happen again when I don't have the time to devote to it to fix any issues quickly. Many people rely on KStars, INDI, and astrometry to do their imaging and I don't want them upset and disappointed, especially if it causes them to lose a whole night of imaging. That isn't to say that I don't work on any projects during the School year, in fact, I certainly do. But I have a bunch of other things to do for KStars as well right now, so I think I should address those things before I start messing with code that is currently working. One option might be to slowly try to submit individual changes/fixes that I had made to the astrometry.net folks any time I get a chance, just going a little at a time, and to not change Stellarsolver at all for now. Another might be to just wait until Summer of 2022 when I have a lot more time.

pemensik commented 2 years ago

I understand it may take time. I tried to move your original work on top of astrometry directory and their changes on astrometry repository history. I guessed from content and minimal diff version 0.76 were used. Then reapplied patches exported from stellarsolver to astrometry branch. Results are on my stellarsolver branch .

If I can find enough time, I will try to split them into general fixes, windows only fixes and functionality changes. Then it might be easier to request just changes needed on unix-like systems to reuse astrometry build directly from stellarsolver. Later to make possible at least limited windows builds directly from astrometry sources.

rlancaste commented 2 years ago

Yes, it is definitely Astrometry 0.76 that we used to base it on. Note that Jasem just made a bunch of changes because apparently something just changed on Windows and a file we were including can't be used any more on windows, so he found a way around that. Was that included in your branch?

pemensik commented 2 years ago

I have attempted to include all changes applied to astrometry subdirectory in current and two previous location. I may have made some errors, because I have removed few commits applying some changes and deleting them in the next commit again. I may have been not successful when applying all patches.

Did you mean commit (https://github.com/pemensik/astrometry.net/commit/cad3375451ff01c576559ab4b386322eeab7d9b5) ?

pemensik commented 2 years ago

Are there any minimal samples, on which I could test my changes still work as they should? Is there some input archive, which has small size and relative simple output, which could be used as kind of unit test?

I understand the code, but know very little about plates solving, how they are obtained and what are correct sources for them. Is there something like Vega start, used as starting point for lame astronomers? I have prepared some changes in branches (https://github.com/pemensik/astrometry.net/tree/stesol-functional and https://github.com/pemensik/stellarsolver/tree/astrometry-reuse). I attempt preparing extra include for WIN32 platform, which would supply missing calls on Windows. It should allow less changes to library, while it should allow compilation on windows. Of course some function would always return failure, but those are not used by stellarsolver.

pemensik commented 2 years ago

Would you mind @rlancaste if I create PR containing some of your fixes to astrometry.net repository? I have selected short set of changes fixing error path leaks. Stored in branch https://github.com/pemensik/astrometry.net/tree/stellarsolver-leakfixes

rlancaste commented 2 years ago

Yes, that was the commit that I meant. Good.

As for testing, the main way that I have tested everything is by just plate solving the same test images under different conditions. I have a tester program in StellarSovler that I wrote that I have used to test all the code many many times. I compare the reliability of the plate solve solutions. I compare the time it takes to solve. I also have looked at it with performance testing software that looks for leaks and warning and makes other suggestions. I compare how native astrometry.net solves it as compared to my build, as compared to ASTAP. But no I don't have unit tests or anything. But I regularly have tested with real photos that I and others have taken on many different systems and simulator images generated by KStars as well.

Sorry I haven't had a chance to look at what you did with it yet. My time for coding has been more limited recently as I said. But by all means if you see any ways that you can easily and simply commit some changes to astrometry.net and it helps to fix up some of the leaks that I found and they accept it and it improves astrometry.net, then that is awesome! I won't necessarily say that every change I made was the right thing to do or the best way to do it. If you are better with the code, then thats great. As I said, I carefully flagged each change I made.

rlancaste commented 2 years ago

Were you able to submit the changes to astrometry.net? It looks like you had taken a few of the leak fixes that I had found with Xcode etc (and maybe some of your own) and put them in a set of commits in your astrometry fork. Did you manage to submit them?