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

Transit analysis addition #142

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tomshaffner
Copy link
Contributor

@tomshaffner tomshaffner commented Jan 7, 2025

I had posted a question at #141 about getting a consolidated report that shows both natal and transit information together, including the aspects/interactions between them. The more I looked the more I thought most of the parts to put this together already existed; this PR is my attempt to do that.

Commit 1 is an addition of a new transit_analysis.py enabling transit analysis in a way that looks at transit and natal together.

Commit 2 is then an addition of a new class to the report.py file that enables reporting on this new TransitAnalysis object.

If I missed something or broke a paradigm somewhere feel free to make changes/tweaks as needed to correct.

@g-battaglia
Copy link
Owner

g-battaglia commented Jan 7, 2025

Hi, thanks again for this contribution as well!
I took a quick look at the pull request and will try to review it in more detail as soon as possible.

In the meantime, I must say I really like the department—well done! However, there’s one thing I’m unclear about:
Does the transit_analysis module and its corresponding class serve only for transit reporting? Because, the way the data is structured, there’s a lot of overlap with what can already be obtained from the data classes (based on Pydantic models) in the kr_models module. Ideally, it would be best to leverage what’s already available from this perspective if possible.

The only missing piece is the ascendant as a standalone KerykeionPointModel, but that should already be resolved by #138, which I still need to review and merge at the moment (I’ll try to do this as soon as possible).

PS: I’d like to add again that the transit report looks truly great!

@tomshaffner
Copy link
Contributor Author

tomshaffner commented Jan 7, 2025 via email

@tomshaffner
Copy link
Contributor Author

Actually, I had some time so went ahead and tried to migrate this to use the existing data models. I left the commits separate because I think anyone trying to understand this might benefit from seeing the separate history, but the end result hopefully is better aligned and more integrated with the existing library.

@g-battaglia
Copy link
Owner

g-battaglia commented Jan 9, 2025

Hmm, I think we can leverage what's already in place to simplify the code further. Anyway, thanks a lot again—great work! In the next few days, I'll put it on a separate branch and try to optimize it myself

@tomshaffner
Copy link
Contributor Author

Feel free to skip the PR and develop further on the same branch then if you want; I'll stop here and look forward to using it soon when it's released!

@tomshaffner
Copy link
Contributor Author

@g-battaglia, I'm going to start using this soon to develop against for a project that pulls from it, you have any timeline on your merging process? If it'll be soon I'd love to align to what you merge into the official library and might try to rework my project to delay a bit in that case, but if it'll take a while to get to maybe better if I fork my own version for now and then hope to merge to the official branch, and any changes it involves, at some future point.

@g-battaglia
Copy link
Owner

I would have liked to have merged everything already, but unfortunately, I haven't had time :/ There are a couple of small changes I'd still like to make (more about keeping the project organized, not functional), and I'll try to do them in the next few days.

However, an important point I need to ask for the merge: can you integrate some unit tests? They would be essential, at least for the analysis part but preferably for the report as well.

@g-battaglia
Copy link
Owner

g-battaglia commented Jan 21, 2025

@tomshaffner let me know if your planning to integrate the unit tests. If not no problem I'll do it myself

@tomshaffner
Copy link
Contributor Author

Sorry, wanted to but haven't had time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants