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

side option to define minute bounds / test suite overhaul #71

Merged
merged 26 commits into from
Oct 4, 2021
Merged

Conversation

maread99
Copy link
Collaborator

@maread99 maread99 commented Jul 31, 2021

@gerrymanoim, Hi Gerry, I feel the time's right for a review of the side option PR before I plough on any further!

This draft PR includes the side implementation in full and a proposal for an overhaul of ExchangeCalendarTestBase.

Of the three commits:

  • Add side parameter to ExchangeCalendar offers the implementation in full.
    • With this implementation all methods that concern minutes (minute_to_session_label, previous_minute, minutes_for_session etc) will work off trading minutes as determined by the side passed to the constructor.
  • Update 24 hour calendars to true 24 hours.
    • Times changed (on calendars and corresponding answers csv files) so that open and close times are the same and offset defined as applicble. Valid side options for these calendars is limited to "left" or "right" in order that every trading minute belongs to one session only.
  • New ExchangeCalendarTestBaseProposal offers the proposed overhaul of ExchangeCalendarTestBase...

(you'll notice I've pushed these commits to a 'side' branch and set up this draft PR from there).

Rather than replace the existing ExchangeCalendarTestBase, at this stage I've simply added a new ExchangeCalendarTestBaseProposal to the end of test_exchange_calendar.py and implemented it only to the test modules of the following selected calendars:

  • 'XLON' (no breaks)
  • 'XHKG' (breaks)
  • '24/7' (24h continuous)
  • '24/5' (24h but not continuous)
  • 'CMES' (24h with sessions crossing UTC midnight)

ExchangeCalendarTestBaseProposal includes only the following methods for now (enough I hope to give you an appreciation of the proposed implementation):

  • test_all_minutes - new test to test trading mintues at sessions' bounds.
  • test_minute_to_session_label - replacement for existing test.
  • test_minute_index_to_session_labels - replacement for existing test.
  • test_is_open_on_minute - replacement for existing test.
  • test_invalid_input - new test to test that constructor raises expected errors on invalid inputs.

Main features of the proposed approach:

  • Creates an Answers class that, from interrogation of the corresponding csv file, provides inputs and expected return values for methods under test. Importantly, it is not intended that this class does more than minimial evaluations. It should not in any way 'repeat the code' of the methods being tested. It does the same kind of work currently done ad-hoc within test methods.
    • Answers class can take a side parameter that provides for its properties to reflect a specific side option.
  • ExchangeCalendarTestBaseProposal provides a parameterised pytest fixture all_calendars_and_answers. Any test that inherits this fixture will be exectuted with a calendar / answers combo for each side option.
  • Moves fully to native pytest framework.

If you were to be happy with the general approach then my intenion would be to:

  • Tidy and speed up what's there
  • Extend ExchangeCalendarTestBaseProposal to cover all tests of ExchangeCalendarTestBase and eventually replace it.
  • Review/revise all calendar-specific test modules to implement new test base.

I look forward to hearing your thoughts!

NB with the exception of the 24 hour calendars the default side is "both" which fully replicates the existing behaviour. Accordingly all tests for non-24hour calendars continue to pass. I've skipped the standard ExchangeCalendar tests for the 24h calendars - some of these tests will fail given that these calendars are, intentionally, not acting as if side were "both" (i.e. the behaviour that the tests were designed for).

Adds side parameter to ExchangeCalendar to define the
bounds of sessions´ trading minutes.

updates ExchangeCalendar.minute_index_to_session_labels.
updates ExchangeCalendar.minute_to_session_label.
updates ExchangeCalendar.is_open_on_minute.

Also:
adds NoSessionsError.
Updates 24 hour calendars to have:
  same open time and close time.
  day offset as applicable.
  valid sides options as only 'left' or 'right'.

Updates 24 hour calendar csv files to reflect new times.

Introduces ExchangeCalendar.default_side. "right" for 24 hour
calendars, "both" for all others.
Proposes a basis for a new ExchangeCalendarTestBase which
accommodates testing ExchangeCalendar methods for
calendars based on all valid 'side' options.

ExchangeCalendarTestBaseProposal includes a limited number
of tests to show proposed implementation.

Answers class provides input for and expected returns from
ExchangeCalendar test methods.

Incorporated to '24-5', '24-7', 'CMES', 'XLON' and 'XHKG'.

Native pytest.
@maread99 maread99 added the enhancement New feature or request label Jul 31, 2021
@maread99
Copy link
Collaborator Author

maread99 commented Jul 31, 2021

TODO

Tests need speeding up.

  • Make sure existing caches working as expected.
  • Make sure ExchangeCalendarTestBaseProposal fixtures being created once-per-class as expected and required (rather than being created anew for every test).
  • Investigate swapping Answer's minute generators (trading_minutes, non_trading_minutes, break_minutes) for pre-defined lists created once and cached.
  • Consider extending sessions covered by minute lists (trading_minutes, non_trading_minutes, break_minutes) to all answers' sessions - how much longer would the test suite take to complete? (went the other way, focusing on quality of tests rather than blanket coverage.)
  • Consider stripped down versions of the minute generators to offer only the minutes without the ancillary objects that offer related sessions. Offer minute generators that iterate over caches to offer 'minute only' (without ancillary objects).
  • The ExchangeCalendarTestBaseProposal tests for '24/5' calendar seem to be running disproportionately quick relative to those for 'XLON' (even after accounting for fewer sessions on the 24/5 answers). Investigate.
  • Alongside Answers.get_next_sessions offer a simpler, faster, Answers.get_next_session that can be used when require only next session.

Then (EDIT - FOLLOWING PROCESS MOVED TO / SUPERSEDED BY (#86))

  • Move forwards with incorporating all existing tests of ExchangeCalendarTestBase to ExchangeCalendarTestBaseProposal.
  • Ensure documentation within test_exchange_calendar.py covers instructions and considerations for defining a calendar test class.
  • Update What times are considered open and closed? section of FAQs.
  • Merge PR to date.
  • PR to replace ExchangeCalendarTestBase with ExchangeCalendarTestBaseProposal for the first five calendar test modules to which proposed base already incorporated (to include moving over calendar-specific tests).
  • Move all calendar test modules over to new test base (over various PRs).
  • PR new minute-related methods.
  • Review whether to move *minute_nanos(side=None) methods , which can currently override default side, to properties that return for default side only (would require revising minute_index_to_session_labels).

@gerrymanoim
Copy link
Owner

Hey @maread99 - thanks for putting this up and documenting this really well.

I think my high level comments would be:

  1. We should definitely be using an enum rather than strings as input for side. This should eliminate some of the validation code we have in a few places.
  2. I'm possibly a bit worried about doing full array copies in one_minute_* functions, even if that only happens when we break the cache.
  3. There's some functions like *_minute_nanos where I'm unclear why they take a side arg, vs automatically using the side on the class.
  4. Probably need some text in the readme about this (though revamping all the docs is on my todo list)
  5. Happy to do your followup todos over time.
  6. I wonder if there's a better name than side?
  7. I think the new test calendar class is great. Definitely some messy logic inside the test class itself, but it cleans up the user facing side a lot.

@maread99
Copy link
Collaborator Author

maread99 commented Aug 21, 2021

@gerrymanoim, thanks for looking through this and for your comments.

The *_minute_nanos currently take a ‘side’ arg as I anticipate adding a side option to some methods to allow for overriding the default ‘side’ for that call only. Also, there is a method (minute_index_to_session_labels IIRC) which requires being able to treat side as ‘both’ for its own purposes.

I’m not able to reply fully at the moment although will do towards the end of the month when I expect to move this forward and incorporate your suggestions. Cheers.

@maread99
Copy link
Collaborator Author

maread99 commented Aug 31, 2021

@gerrymanoim, thanks again for your comments from which I'm assuming you're happy, subject to the following, for me to progress with the proposed side implementation and the supporting testing overhaul(?).

  1. We should definitely be using an enum rather than strings as input for side. This should eliminate some of the validation code we have in a few places.

👍 Will do.(?) I'm probably missing something, although having looked at what this would involve any net benfit isn't immediately apparent to me(?). Given that the 24 hour calendars can only take a subset of possible side values, I don't think there would be any reduction in validation code. However, wouldn't using enums make usage more complicated by requiring the user to first import the enum rather than simply passing through a string?

  1. I'm possibly a bit worried about doing full array copies in one_minute_* functions, even if that only happens when we break the cache.

I'm assuming the concern here is memory? The arrays received are always limited in size (to the number of the calendar's days) and the copy is then worked on directly to create the return, i.e. there's no intermediary object taking up memory.

  1. There's some functions like *_minute_nanos where I'm unclear why they take a side arg, vs automatically using the side on the class.

I anticipate adding a side option to some methods to allow for overriding the default side for that call only. minute_index_to_session_labels already includes this option which facilitates the evalution of _late_opens and _early_closes by the constructor (which calls minute_index_to_session_labels with side as 'both' in order to map open / close times to session labels).

  1. Probably need some text in the readme about this (though revamping all the docs is on my todo list).

👍 I'll update the What times are considered open and closed? section of FAQs.

  1. Happy to do your followup todos over time.

I hope to get through all of this over the next couple of weeks. To aid reviews I'll work on the side branch with sequential commits. To avoid conflicts with recent and any future merged PRs, I might request an intermediary merge before moving the calendar test modules to the new test base.

  1. I wonder if there's a better name than side?

😄. I did give it some thought before settling on 'side'. I originally had it as the arguably worse 'closed'. I guess that, with usage, parameter names take on the significance of the functionality they provide. Of course, more than open to suggestions for an alternative name that might better indicate what the option does.

  1. I think the new test calendar class is great. Definitely some messy logic inside the test class itself, but it cleans up the user facing side a lot.

My intenion is to keep all the critical evaluations (test inputs / expected outputs) within Answers rather than scattered around the TestBase.

Revisons to testing to increase speed and coverage.
adds ExchangeCalendar.is_trading_minute.
adds ExchangeCalendar.is_break_minute.
updates ExchangeCalendar.is_open_on_minute.
Extends ExchangeCalendarTestBaseProposal to include (and supporting
fixtures):
  test_calculated_against_csv
  test_bound_start
  test_bound_end
  test_sanity_check_session_lengths
  test_adhoc_holidays_specification
  test_prev_next_open_close

Extends Answers to support newly included tests.

Adds parsing and bounds handling to ExchangeCalendar
methods previous_open, previous_close, next_open, next_close.
Reduces setup time of cached properties.
Extends ExchangeCalendarTestBaseProposal to include following
methods:
  test_next_prev_minute

Extends Answers to support newly included test.

Adds parsing and bounds handling to ExchangeCalendar
methods previous_minute and next_minute.
Removes _minute_to_session_label_cache from ExchangeCalendar.
@gerrymanoim
Copy link
Owner

@gerrymanoim, thanks again for your comments from which I'm assuming you're happy, subject to the following, for me to progress with the proposed side implementation and the supporting testing overhaul(?).

Yep - I think there's a lot here, but overall everything is reasonable.

  1. We should definitely be using an enum rather than strings as input for side. This should eliminate some of the validation code we have in a few places.

👍 Will do.(?) I'm probably missing something, although having looked at what this would involve any net benfit isn't immediately apparent to me(?). Given that the 24 hour calendars can only take a subset of possible side values, I don't think there would be any reduction in validation code. However, wouldn't using enums make usage more complicated by requiring the user to first import the enum rather than simply passing through a string?

That's a fair point, I think my instinct is to just use an enum where string constants are needed and do isinstance checks for those constants. If we needed to use side outside the core class or were more strictly type checking things, I think I'd be more insistent on using enums.

  1. I'm possibly a bit worried about doing full array copies in one_minute_* functions, even if that only happens when we break the cache.

I'm assuming the concern here is memory? The arrays received are always limited in size (to the number of the calendar's days) and the copy is then worked on directly to create the return, i.e. there's no intermediary object taking up memory.

Mostly performance actually, but I agree with your point that these are generally of a limited size so it should be fine.

  1. There's some functions like *_minute_nanos where I'm unclear why they take a side arg, vs automatically using the side on the class.

I anticipate adding a side option to some methods to allow for overriding the default side for that call only. minute_index_to_session_labels already includes this option which facilitates the evalution of _late_opens and _early_closes by the constructor (which calls minute_index_to_session_labels with side as 'both' in order to map open / close times to session labels).

What would be the benefit for these other methods?

  1. Probably need some text in the readme about this (though revamping all the docs is on my todo list).

👍 I'll update the What times are considered open and closed? section of FAQs.

  1. Happy to do your followup todos over time.

I hope to get through all of this over the next couple of weeks. To aid reviews I'll work on the side branch with sequential commits. To avoid conflicts with recent and any future merged PRs, I might request an intermediary merge before moving the calendar test modules to the new test base.

Yep - that's fine - don't want you to have to keep having issues with other PRs coming in.

  1. I wonder if there's a better name than side?

😄. I did give it some thought before settling on 'side'. I originally had it as the arguably worse 'closed'. I guess that, with usage, parameter names take on the significance of the functionality they provide. Of course, more than open to suggestions for an alternative name that might better indicate what the option does.

Yep I also can't think of anything particularly better here.

  1. I think the new test calendar class is great. Definitely some messy logic inside the test class itself, but it cleans up the user facing side a lot.

My intenion is to keep all the critical evaluations (test inputs / expected outputs) within Answers rather than scattered around the TestBase.

Extends ExchangeCalendarTestBaseProposal to include following
methods:
  test_next_prev_session
  test_date_to_sesion_label
  test_minutes_in_range

Extends Answers to support newly included test.

Adds parsing and updates documentation for ExchangeCalendar
methods next_session_label, previous_session_label,
date_to_session_label, minutes_in_range.
Orders methods into sections.

Identifies those properties that should, and should not,
be overriden by a subclass to define a calendar.
Adds ExchangeCalendar minute properties/ methods:
  first_minutes
  last_minutes
  last_am_minutes
  first_pm_minutes
  session_first_minute()
  session_last_minute()
  session_last_am_minute()
  session_first_pm_minute()

Adds tests to ExchangeCalendarTestBaseProposal:
  test_minutes_properties
  test_session_minute_methods

Renames Answers properties:
  first_trading_minutes* to first_minutes*
  last_trading_minutes* to last_minutes*
  last_am_trading_minutes* to last_am_minutes*
  first_pm_trading_minutes* to first_pm_minutes*
@maread99
Copy link
Collaborator Author

@gerrymanoim, thanks for your further comments.

There's some functions like *_minute_nanos where I'm unclear why they take a side arg, vs automatically using the side on the class.

I anticipate adding a side option to some methods to allow for overriding the default side for that call only. minute_index_to_session_labels already includes this option which facilitates the evalution of _late_opens and _early_closes by the constructor (which calls minute_index_to_session_labels with side as 'both' in order to map open / close times to session labels).

What would be the benefit for these other methods?

My own usage of exchange calendars led me to accommodate this functionality - very occassionally I found a need to treat 'side' differently from the default for a specific purpose (similar to how minute_index_to_session_labels does). However, I've yet to convince myself of the need for this. I also don't like the *_minute_nanos being defined as functions rather than properties. After this PR is resolved (together with subsquent PRs moving calendars to the new test base) I have a few new methods to propose, all based on the concept of trading minutes. I would like to leave the *_minute_nanos methods as they are until then. I'll then reassess whether their accommodating a 'side' argument can be justified (added to the todo above). If it can't, I'll make them properties and revise minute_index_to_session_labels. Does this sound reasonable?

Yep - that's fine - don't want you to have to keep having issues with other PRs coming in.

👍 I anticipate having the new test base proposal completed this week, at which point this will be good for reviewing / merging. I'll offer a summary of changes made and what will be left to do over subsequent PRs.

@gerrymanoim
Copy link
Owner

Yep - sounds good to me.

Reorders test methods of ExchangeCalendarTestBaseProposal to
follow order of corresponding methods of ExchangeCalendar.
@maread99 maread99 mentioned this pull request Sep 14, 2021
49 tasks
Extends ExchangeCalendarTestBaseProposal to include following
methods:
  test_minutes_for_session
  test_minutes_for_sessions_in_range

Extends Answers to support newly included tests.

Revises, adds parsing and updates documentation for ExchangeCalendar
methods:
  minutes_for_session
  minutes_for_sessions_in_range

Adds deprecate decorator (for deprecating ExchangeCalendar methods).

Deprecates following methods from ExchangeCalendar and moves
functionality exclusively to QuantopianUSFuturesCalendar:
  execution_minutes_for_session
  execution_minutes_for_sessions_in_range
  execution_time_from_open
  execution_time_from_close
Adds following methods to ExchangeCalendarTestBaseProposal:
  test_start_end
  test_daylight_savings
  test_has_breaks
  test_session_has_break
  test_minutes_count_for_sessions_in_range (not previously tested)

Adds daylight_saving_days fixture to ExchangeCalendarTestBaseProposal.

Extends Answers to support new tests.

Adds parsing and updates documentation for ExchangeCalendar methods:
  has_breaks
  session_has_break

Rewrites `ExchangeCalendar.minutes_count_for_sessions_in_range`.
@maread99 maread99 mentioned this pull request Sep 17, 2021
10 tasks
Adds tests to ExchangeCalendarTestBaseProposal for previously untested
methods/properties:
  `test_all_sessions`
  `test_opens_closes_break_starts_ends`
  `test_calendar_bounds_properties`
  `test_late_opens`
  `test_early_closes`
  `test_is_session`
  `test_minutes_window`

Adds fixtures to ExchangeCalendarTestBaseProposal:
  late_opens
  early_closes

Extends Answers to support new tests.

Adds parsing (if applic) and updates documentation for ExchangeCalendar
properties/methods:
  `opens`
  `closes`
  `break_starts`
  `break_ends`
  `late_opens`
  `early_closes`
  `is_session`
  `minutes_window`

Fixes `ExchangeCalendar.minutes_window` (forward looking window
would start prior to `start_dt` if `start_dt` not a trading minute).
Implemented to have same behaviour as `sessions_window`.

Adds to calendar_helpers.py:
  `TradingMinute` type
  `parse_trading_minute` function

Adds `test_parse_trading_minute` test to `test_calendar_helpers.py`.

Adds `NotTradingMinuteError` to errors.py.
@maread99 maread99 changed the title side option to define sessions' trading minute bounds side option to define minute bounds / test suite overhaul Sep 17, 2021
@maread99
Copy link
Collaborator Author

@gerrymanoim, I think I'm there, with this initial PR ready to merge pending your review.

It's ended up as quite a bit more than implementing side. As a result of working through the test base I ended up undertaking a general review of both ExchangeCalendar and the tests (excluding calendar-defintion methods). All changes are hopefully documented to the corresponding commit messages. In summary:

  • Added tests for ExchangeCalendar methods/properties that were not previously tested for (including minutes_count_for_sessions_in_range, is_session, minutes_window, late_opens, early_closes...)
  • Added (where missing) parsing, annotations and standardised documentation for all ExchangeCalendar methods and properties (except those primarily concerned with calendar definition/construction).
  • Added out-of-bounds handling to some ExchangeCalendar methods (including sessions_window, minutes_window)
  • Reordered methods of ExchangeCalendar (to labelled sections) - new test base class follows the same order.
  • Proposed renaming some methods and parameters for reasons for consistency. The whys and whats are covered between Renaming of some ExchangeCalendar methods #85 (method names) and todo for release 4.0 #61 (param names).

Testing

Continuing from my earlier notes...

Generally tests have been made more comprehensive as a result of:

  • including out-of-bounds inputs.
  • including edge cases around start/end of breaks.
  • considering 'gaps' around sessions on a per-session basis as opposed to the previous binary treatment at a class level. For example, the Monday/Friday sessions of the 24hr calendars '24/5' and 'CMES' are now treated as having gaps before/after rather than being assumed as gapless. Accordingly these calendars are subjected to tests that they weren't previously.
  • including non-session date inputs to methods that evaluate or interrogate a range of sessions where the range bounds can be defined by a session or non-session date (for example, sessions_in_range, session_distance). Previously such methods were tested only with sessions as inputs.
  • definition of Answers.session_blocks where a block is a contiguous run of sessions that represents a unique circumstance, for example "next_close_later", "next_break_end_earlier", "with_break_to_without_break", "without_gap_to_with_gap", "follows_non_session" etc. The existing test base seems a bit feast or famine - either blanket testing or limiting testing to a single unusual session block. The new base aims for somewhere in between, subjecting all methods a wide variety of circumstances without testing beyond what might be considered reasonably necessary. The focus is very much on edge cases / just within edge cases.

Speed

I anticipate that the new test suite will run somewhat slower than the existing, possibly around 1.7x, maybe less. This is partly due to the more comprehensive testing and partly due to methods that are dependent on the evaluation of minutes now being tested against four versions of each calendar (one for each side).

Other points of note

  • The new test class does away with the notable majority of the class attributes that contributors were requried to define (for example HAVE_EARLY_CLOSES, TEST_START_END_*, GAP_BETWEEN_SESSIONS etc). The need for these has been obviated by evaluating properties directy from the answers csv. Where these attributes are still required they are defined as fixtures. Only two are mandatory (calendar_cls, max_session_hours) and only two others may be requried if the calendar strays from the default (start_bound and end_bound).

  • an internal _parse argument provides for internal calls and testing to skip parsing.

  • test_exchange_calendar.py will clean up somewhat with:

    • removing the existing ExchangeCalendarTestBase when all the calendar test modules have been moved over to the new base (should also allow for losing unittest and some other imports).
    • cleaner caching (@functools.cached_property) when min supported python version moves to 3.8.

Moving On

I've laid out in #86 the path to moving calendars over to the new test base. I envisage this ideally being followed by some method renaming (#85 - what do you reckon?), a revised README and culminating with cutting 3.4(?).

@maread99 maread99 marked this pull request as ready for review September 17, 2021 22:59
Updates calendar_helpers.py parse_timestamp function to handle input
with a non-zero second (or more accurate) component.
@maread99
Copy link
Collaborator Author

I've added one small commit to cover something that became apparent to me when working on documentation.

@gerrymanoim, you might want to check out the proposed doc - it ties together the side option covered by this PR and the proposed renaming (#85). It's on the 'readme' branch (I've overhauled the README and added a couple of .ipynb tutorials).

@maread99
Copy link
Collaborator Author

@gerrymanoim, although I've added another small commit this is no longer a WIP (at least as far as this PR is concerned) and is good to review / merge as and when you're in a position to look at it...

@gerrymanoim
Copy link
Owner

Awesome! Will take a look in the next few days.

@maread99
Copy link
Collaborator Author

Great! Be sure to check out the proposed doc on the 'readme' branch - README overhaul and 4 ipynb tutorials. It's intended to be published alongside release 3.4 (assumes this PR is merged and that the renaming covered in #85 happens).

This was referenced Sep 23, 2021


@pytest.mark.skip
Copy link
Owner

Choose a reason for hiding this comment

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

Is this purposely skipped?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I included a note on this at the end of the first comment.

Basically, introducing the side option has allowed the 24 hour calendars to be truly 24 hour - the open times have been moved 'back' one minute to represent the actual open time. These calendars can only take side as "left" or "right". They default to "right" to maintain the original behaviour; each minute remains associated with the same session as perviously. Whilst the behvaiour hasn't changed (save for a change to the open time) some of the old tests fail as they were designed on the assumption that the open minute and close minute would both be a trading minutes - this assumption continues to hold for all the other calendars (which default to side "both") but not the 24 hour ones.

All the 24 hour calendars are subjected to and pass the new test suite.

@@ -532,71 +575,253 @@ def special_offsets_adhoc(self):
"""
return []

# -----
# ------------------------------------------------------------------
# -- NO method below this line should be overriden on a subclass! --
Copy link
Owner

@gerrymanoim gerrymanoim Oct 2, 2021

Choose a reason for hiding this comment

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

If you wanted to, we could enforce this via an __init_subclass__ checking this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wouldn't want to impede users doing what they want with their own calendars. I've added a test test_base_integrity to ensure the integrity of the pre-defined ones.

@@ -250,13 +333,21 @@ def __init__(self, start: Date | None = None, end: Date | None = None):
self.last_trading_session = _all_days[-1]

self._late_opens = pd.DatetimeIndex(
_special_opens.map(self.minute_index_to_session_labels)
_special_opens.map(lambda x: self.minute_index_to_session_labels(x, "both"))
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this set to both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It shouldn't be! _special_opens should map to sessions based on minutes evaluated for side "left" and _special_closes should map to sessions based on minutes evaluated for side "right". It could be left as "both" if not for the 24 hour calendars (which don't have a "both" option). I had it as "both" simply to maintain the original behaviour - the only reason this didn't result in a bug is that no existing 24 hour calendar has a late open. I would now change these to "left" and "right", but...

Having looked into what's going on here the whole mapping appears to be superfluous. _late_opens as evaluated is actually already represented by _special_opens.index and _early_closes by _special_closes.index, i.e. the assignment line under review could be replaced with a simple:

self._late_opens = _special_opens.index

I'm not sure where the need for the method call came from? Perhaps _special_opens was originally defined differently? In any event, I'll add a commit to change these assignments to the direct assignment (as above). Let me know if you think the mapping is necessary and that I'm missing something here.

NB a consequence of losing the call to minute_index_to_session_labels is that it's more likely that the *_minute_nanos methods will end up being changed to properties (item 5 on #86).

@gerrymanoim
Copy link
Owner

Some minor questions/comments, but LGTM. Feel free to merge when you're ready.

The new notebooks are great! As an FYI github will render the notebooks directly, but I don't have view on linking directly vs nbviewer.

@maread99
Copy link
Collaborator Author

maread99 commented Oct 4, 2021

Great. I'm going to add a commit or two to cover the issues / points you've raised and then I'll merge and move forwards with moving the test modules over to the new suite.

The new notebooks are great! As an FYI github will render the notebooks directly, but I don't have view on linking directly vs nbviewer.

Glad you like them. I just prefer the nbviewer rendering. The only issue is that (I believe) it'll be necessary to update the nbviewer links on the README every time the notebooks are updated (I might yet end up linking to the GitHub pages).

Updates evaluation of `_late_opens` and `_early_closes`.
Adds test `test_base_integrity` to ensure a subclass does not override
base properties or methods that are not intended to be overriden.
@gerrymanoim
Copy link
Owner

The only issue is that (I believe) it'll be necessary to update the nbviewer links on the README every time the notebooks are updated (I might yet end up linking to the GitHub pages).

We can always transition to sphinx in the future

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

Successfully merging this pull request may close these issues.

2 participants