ENH: Add Korea Exchange (XKRX) #41
Conversation
There was a problem hiding this comment.
@freddiev4 Overall looks good, just left a few small questions.
| the country of South Korea. | ||
|
|
||
| Open Time: 9:00 AM, KST (Korean Standard Time) | ||
| Close Time: 3:30 PM, KST (Korean Standard Time) |
There was a problem hiding this comment.
I might also just add a comment saying they observe standard time all year round (i.e. no daylight savings time).
tests/test_xkrx_calendar.py
Outdated
| self.assertNotIn(session_label, self.calendar.all_sessions) | ||
|
|
||
| def test_constrain_construction_dates(self): | ||
| # the XKRX calendar currently goes from 1999 to 2019, inclusive. |
|
|
||
| Due to the complexity around the Korean holidays, we are hardcoding | ||
| a list of holidays covering 1986-2019, inclusive. | ||
| """ |
There was a problem hiding this comment.
I don't know if it's worth it, but maybe you could add the list of Korean holidays in this docstring, like is done in other calendars? Just the names of them.
|
|
||
| # Korea exchange is open from 9am to 3:30pm | ||
| MAX_SESSION_HOURS = 6.5 | ||
| HAVE_EARLY_CLOSES = False |
There was a problem hiding this comment.
I see there are no early closes, but are there any holidays that may or may not fall on a weekend? In which case we would need to test that those holidays are either not observed some years, or properly roll to the next weekday? I'm just trying to think of any edge cases we might want to specifically test here, other than just a "normal" year.
There was a problem hiding this comment.
I think there are at least two holidays that I can think of that can occur on a weekend.
Is there anywhere we already have a test like that?
There was a problem hiding this comment.
Here is an example of holidays falling on weekends that are made up: https://github.com/quantopian/trading_calendars/blob/master/tests/test_xasx_calendar.py#L42
Here is an example of holidays falling on a weekend that are not made up: https://github.com/quantopian/trading_calendars/blob/master/tests/test_xhel_calendar.py#L45
|
@dmichalowicz updated the PRs with your feedback |
| - Christmas Day | ||
| - End of Year Holiday | ||
|
|
||
| NOTE: Hangeul Day became a national holiday in 2013 |
There was a problem hiding this comment.
Could you add a short test asserting that Hangeul Day (October 9th) is a session in 2012 but not a session in 2013?
There was a problem hiding this comment.
Sure! fwiw I think it should be the other way around; Hangeul Day should not be a session in 2012, and only starts to be a Market-closing Holiday in 2013
tests/test_xkrx_calendar.py
Outdated
|
|
||
| # Holidays below falling on a weekend should | ||
| # be made up during the week. | ||
| expected_sessions = [ |
There was a problem hiding this comment.
I think you meant to call this expected_holidays?
tests/test_xkrx_calendar.py
Outdated
| # not be made up during the week. | ||
| expected_holidays = [ | ||
| # National Foundation Day on a Saturday | ||
| T('2015-10-03'), |
There was a problem hiding this comment.
Since this is a Saturday (which is always not a session), I would actually instead test that the surrounding Friday and Monday are sessions (i.e. not holidays).
freddiev4
force-pushed
the
annyeonghaseyo
branch
from
46e3aac to
a9d29e3
Compare
This PR adds the Korea Exchange (XKRX) and uses a trusted source for the holidays up until the end of 2019.
@dmichalowicz could you take a look at this when you have time? Opening Zipline / other PRs as well.