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

Improve XKRXExchangeCalendar #19

Merged
merged 8 commits into from
Jun 20, 2021
Merged

Conversation

elbakramer
Copy link

Core changes in ExchangeCalendar

XKRXExchangeCalendar specific changes

Minor changes

@gerrymanoim
Copy link
Owner

gerrymanoim commented Apr 30, 2021

Apologies - I know I owe you actual feedback here but have been quite busy lately. Trying to check in and keep the CI running though.

@elbakramer
Copy link
Author

elbakramer commented Apr 30, 2021

It's OK.
Thanks for the heads up.

I just figured out that I also can enable the Github Actions in my fork and run CI tests on push.
So I'll let you know when all failing checks are fixed.

It seems that I'm still missing many checks btw...

@elbakramer
Copy link
Author

Finally all checks have passed in my fork: https://github.com/elbakramer/exchange_calendars/runs/2479460122

Copy link
Owner

@gerrymanoim gerrymanoim left a comment

Choose a reason for hiding this comment

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

I took a quick look at these changes today. Thanks for all the work! Very impressive. I left a few quick notes, but in general I think this looks reasonably good.

I'm a bit wary of subclassing the pandas Holiday/Businessday/Calendar objects (both from a maintainability and using this library in other libraries perspective) but I think I'm okay using it on just the XKRX calendar and what you've done seems reasonable. I'd like to take a deeper look at those a bit later though.

exchange_calendars/pandas_extensions/holiday.py Outdated Show resolved Hide resolved


# Automatically generated holidays using /etc/update_xkrx_holidays.py script.
# Note that there are some missing holidays compared to the original holidays.
Copy link
Owner

Choose a reason for hiding this comment

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

Any idea why they're missing?

Copy link
Author

@elbakramer elbakramer May 5, 2021

Choose a reason for hiding this comment

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

Not much really.

The following page offers holidays ranging from current year to year 2009:
http://global.krx.co.kr/contents/GLB/05/0501/0501110000/GLB0501110000.jsp
And it seems that those holidays are obviously valid with no missing holidays.

While it might seem that we can only check the holidays until year 2009 in the page,
that page's underlying API even gives responses with years older than 2009, the limitation in UI.
So I just blindly dumped the responses from that API as much as I can get and combined them with the existing ones.

I expected that it had to be the single source of truth but just turned out that it was not, with some missing holidays.
Since this is not an desirable usage in their perspective, I think we cannot expect any of integrity in data beyond year 2009.

exchange_calendars/exchange_calendar_xkrx.py Show resolved Hide resolved
exchange_calendars/exchange_calendar_xkrx.py Show resolved Hide resolved
@gerrymanoim
Copy link
Owner

Apologies - I'll take a look again and get this merged this weekend.

@elbakramer
Copy link
Author

It's OK. Take your time.

@gerrymanoim gerrymanoim merged commit 2acb51c into gerrymanoim:master Jun 20, 2021
@gerrymanoim
Copy link
Owner

@elbakramer - Thanks for all the work on this. Merged now.

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