adding semester_dates function #58

shex1627 · 2017-03-12T20:05:53Z

A utility function for the semester job distribution chart.

1.I thought of making spring_start and fall_start constants but "year" would be a variable that later I have to fix, which I think will make the code harder to understand unless you guys have some better solutions.

abizer

Why subtract one day and why switch the order if it's before August?

shex1627 · 2017-03-12T21:17:35Z

So the logic is:
from 01/01 to 07/31 is considered "spring"
and 08/01 to 12/31 is considered "fall"

Basically you have one day, and you expand that day to the semester start and semester end. Subtracting one is to make sure the fall and spring don't overlap.

abizer · 2017-03-12T21:33:01Z

Oh ok that makes sense

…

On Mar 12, 2017 2:17 PM, "shicheng" ***@***.***> wrote: So the logic is: from 01/01 to 07/31 is considered "spring" and 08/01 to 12/31 is considered "fall" Basically you have one day, and you expand that day to the semester start and semester end. Subtracting one is to make sure the fall and spring don't overlap. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB0_lqyrLeQh5yWtSZuRWlKtcDQLUW4Yks5rlGDvgaJpZM4Map3k> .

kpengboy · 2017-03-13T16:11:02Z

ocflib/lab/stats.py

+    if day.month < 8:
+        return (spring_start, fall_start - one_day)
+    else:
+        return (fall_start, spring_start - one_day)


Shouldn't this be the spring_start of the next year

this is part of the SQLquery for the jobs, so not exactly the academic year.

sorry Kevin I was wrong. Do you have any better way of fixing this?
My current proposal is

if day < fall_start: return (spring_start, fall_start - ONE_DAY) else: return (fall_start, spring_start.replace(year=day.year+1) - ONE_DAY)

kpengboy · 2017-03-13T16:11:37Z

ocflib/lab/stats.py

+        day = date.today()
+    spring_start = date(year, 1, 1)
+    fall_start = date(year, 8, 1)
+    if day.month < 8:


I would've written it if day < fall_start, I think that's more intuitive...

tested, and it works. Originally I was concerned that we can't compare day with fall_start and I was lazy to figure that out.

matthew-mcallister

Seems like a fine change save for some minor things. Please don't merge without rebasing though; if you need help with that, just ask. PROTIP: Never do merge master; always rebase master.

matthew-mcallister · 2017-03-20T04:05:09Z

ocflib/lab/stats.py

@@ -79,6 +79,22 @@ def current_semester_start():
        return today.replace(month=1, day=1)


+def semester_dates(day=None):
+    """Return a tuple (start day, end day) for the current semester.
+    Defaults to today if none is provided.


Put a newline before this line.

which line(number)?

Should be

"""Return a tuple (start day, end day) for the current semester. Defaults to today if none is provided. """

matthew-mcallister · 2017-03-20T04:20:13Z

ocflib/lab/stats.py

+    spring_start = date(year, 1, 1)
+    fall_start = date(year, 8, 1)
+    fall_end = date(year, 12, 31)
+    if day < fall_start:


This will error if you give it a datetime object. Can you replace this with something like

if date(day.year, day.month, day.day) < fall_start:

to implicitly convert datetime to date?

if day.month < fall_start.month:

?

I'd rather not assume it starts/ends on a month if we can avoid it, though that is currently the case.

Good try, but I think @chriskuehl is right.

matthew-mcallister · 2017-03-20T23:08:27Z

Also, can you add a simple test for this? Not really sure what would be good. Maybe just pick a couple dates that you think should be in the same semester and test that they return the same tuple?

shex1627 · 2017-04-10T01:06:46Z

commented so that you guys can see the new commit.

matthew-mcallister · 2017-04-10T02:08:22Z

tests/lab/stats_test.py

+def test_semester_dates():
+    assert semester_dates() is not None
+    assert semester_dates(date(2016, 1, 1)) == (date(2016, 1, 1), date(2016, 7, 31))
+    assert semester_dates(date(2017, 12, 30)) == (date(2017, 8, 1), date(2017, 12, 31))


Thanks for the tests.

matthew-mcallister · 2017-04-10T02:12:20Z

ocflib/lab/stats.py

+    year = [day.year]
+    if [day.month, day.year] < FALL_START:
+        return (date(*(year + SPRING_START)),
+                date(*(year + SPRING_END)))


Why all this list arithmetic? Just make SPRING_START etc. namedtuples or dates and do

return (date(year, SPRING_START.month, SPRING_START.day), date(year, SPRING_END.month, SPRING_END.day))

If I use python Date object then I have to use a dummy attribute 'year' since teach year is difference.

#maybe we will use that in the future DUMMY_YEAR = 1000 SPRING_START = date(DUMMY_YEAR, 1, 1) # ... return (date(year, SPRING_START.month, SPRING_START.day), date(year, SPRING_END.month, SPRING_END.day))

if I use namedtuples then any other files that use those constants may have to import the 'collection' library.

from collections import namedtuple Date_No_Year <- namedtuple('month', 'day') SPRING_START = Date_No_Year(1, 1)

Do you like the dummy_year implementation better or creating a new structure using namedtuple?

I'd say pick one of them and go with it. Both approaches have their own drawbacks.

If you go with the dummy attribute route, note that you can use the replace method on a date object to simplify your code.

If you create a new namedtuple, note that you can do something like this:

return (date(year, *SPRING_START), date(year, *SPRING_END))

kpengboy · 2017-04-10T18:57:22Z

ocflib/lab/stats.py

@@ -79,6 +83,22 @@ def current_semester_start():
        return today.replace(month=1, day=1)


+def semester_dates(day=None):
+    """Return a tuple (start day, end day) for the current semester.


You probably meant "for the semester containing the given day"?

kpengboy · 2017-04-10T18:58:46Z

ocflib/lab/stats.py

+    if day is None:
+        day = date.today()
+    year = [day.year]
+    if [day.month, day.year] < FALL_START:


I think you mean day.day instead of day.year?

kpengboy · 2017-04-10T18:59:40Z

While you're at it, you can rewrite the current_semester_start() function to call semester_dates instead.

shex1627 · 2017-04-10T21:36:28Z

New revision updated. Why is Kevin so good at detecting logical bugs?

shex1627 · 2017-04-16T20:09:07Z

could anyone have a look at this?

matthew-mcallister

Other than the one comment, please make sure to rebase your branch so we don't pull in your merge commit. As a reminder, you should rebase instead of merging to update your own branch.

matthew-mcallister · 2017-04-16T20:46:11Z

ocflib/lab/stats.py

@@ -13,6 +13,14 @@
 # when we started keeping stats
 STATS_EPOCH = date(2014, 2, 15)

+# constants for semesters dates
+# dummy year variable
+DUMMY_YEAR = 1000


I don't think there's any reason to name this constant (and superfluous constants pollute the namespace). How about just leaving a comment? For example,

# The year is a placeholder; we only care about month and day. SPRING_START = date(1, 1, 1)

chriskuehl · 2017-04-16T22:51:17Z

ocflib/lab/stats.py

+SPRING_START = date(DUMMY_YEAR, 1, 1)
+SPRING_END = date(DUMMY_YEAR, 7, 31)
+FALL_START = date(DUMMY_YEAR, 8, 1)
+FALL_END = date(DUMMY_YEAR, 12, 31)


this feels really weird, what about just doing

import functools from datetime import date spring_start = functools.partial(date, month=1, day=1) spring_end = functools.partial(date, month=7, day=31) fall_start = functools.partial(date, month=8, day=1) fall_end = functools.partial(date, month=12, day=31)

then later instead of doing SPRING_START.replace(year=2017) just do spring_start(year=2017)

shex1627 · 2017-04-30T21:51:19Z

@chriskuehl I use your functool solution, but also rename constants to be like FALL_START_FUN so that others know they are not date objects when they want to use them.

matthew-mcallister · 2017-04-30T21:58:49Z

ocflib/lab/stats.py

    else:
-        return today.replace(month=1, day=1)
+        return (FALL_START_FUNP(year=day.year),


This could be fall_start but it doesn't really matter.

chriskuehl · 2017-04-30T22:06:13Z

ocflib/lab/stats.py

+SPRING_START_FUNP = functools.partial(date, month=1, day=1)
+SPRING_END_FUNP = functools.partial(date, month=7, day=31)
+FALL_START_FUNP = functools.partial(date, month=8, day=1)
+FALL_END_FUNP = functools.partial(date, month=12, day=31)


these are functions, why are they uppercase? they're not constants.

you don't need the FUNP suffix to mark them as functions if you just name them like functions :)

although those are functions but I think the names could be misleading for people to think they are date objects. Do we need to do anything about that? Or You don't think it is a problem at all.

I agree the name should indicate that these objects are callable. How about using a verb in the name? E.g. get_fall_start or even fall_start_for_year.

those functions are not callable but simply a function that takes the year as argument to return a date object. I think fall_start_funp is a decent option, at least people would get confused and lookup where the variables are defined.

By that standard, the dummy-year-date-constant implementation is not a bad alternative.

abizer · 2017-04-30T22:32:57Z

rename them entirely to something more indicative of being functions like `get_fall_start` or something like that? On Apr 30, 2017 3:23 PM, "shicheng" <notifications@github.com> wrote: *@shex1627* commented on this pull request. ------------------------------ In ocflib/lab/stats.py <#58 (comment)>:

@@ -13,6 +14,12 @@

# when we started keeping stats STATS_EPOCH = date(2014, 2, 15) +# function partial constants for semesters dates +SPRING_START_FUNP = functools.partial(date, month=1, day=1) +SPRING_END_FUNP = functools.partial(date, month=7, day=31) +FALL_START_FUNP = functools.partial(date, month=8, day=1) +FALL_END_FUNP = functools.partial(date, month=12, day=31) although those are functions but I think the names could be misleading for people to think they are date objects. Do we need to do anything about that? Or You don't think it is a problem at all. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB0_ltiKENcLW86oqs5kvzm_M2APC6B8ks5r1QnpgaJpZM4Map3k> .

shex1627 · 2017-04-30T23:50:20Z

renaming all those function partials to get_xxx_xxx, so are we ready to merge now?

matthew-mcallister · 2017-04-30T23:56:08Z

ocflib/lab/stats.py

+get_spring_start = functools.partial(date, month=1, day=1)
+get_spring_end = functools.partial(date, month=7, day=31)
+get_fall_start = functools.partial(date, month=8, day=1)
+get_fall_end = functools.partial(date, month=12, day=31)


Now that I think about it, these should probably be prefixed with _.

shex1627 · 2017-05-21T19:29:18Z

adding prefix "_" to the functions

matthew-mcallister

Great, I think this is finally ready to be shipped. Unless someone objects, I will squash and merge this today.

Thanks for your work, @shex1627. I know it isn't easy pleasing all these picky reviewers.

shex1627 · 2017-05-24T19:21:21Z

do you guys want me to squash the commits as well

matthew-mcallister · 2017-05-24T19:38:47Z

Oh, it looks like the squash and merge button isn't available. You can if you want, otherwise I will.

shex1627 · 2017-05-24T20:00:08Z

I will mess with my local repo see if I can do it myself first. Can't rely on others every time

shex1627 · 2017-05-26T00:29:51Z

Seems like I did it(squash the commits) guys
Not sure if the standard way
first
git reset --soft HEAD~9 && git commit
git push --force origin semesterJobUtil

matthew-mcallister · 2017-05-26T00:33:45Z

shex1627 requested review from chriskuehl, jvperrin and NickImpicciche March 12, 2017 21:07

abizer reviewed Mar 12, 2017

View reviewed changes

kpengboy reviewed Mar 13, 2017

View reviewed changes

matthew-mcallister requested changes Mar 20, 2017

View reviewed changes

matthew-mcallister reviewed Apr 10, 2017

View reviewed changes

kpengboy reviewed Apr 10, 2017

View reviewed changes

matthew-mcallister requested changes Apr 16, 2017

View reviewed changes

chriskuehl reviewed Apr 16, 2017

View reviewed changes

shex1627 force-pushed the semesterJobUtil branch from 3ef339e to 97fd616 Compare April 30, 2017 21:46

matthew-mcallister approved these changes Apr 30, 2017

View reviewed changes

chriskuehl reviewed Apr 30, 2017

View reviewed changes

matthew-mcallister reviewed Apr 30, 2017

View reviewed changes

matthew-mcallister approved these changes May 24, 2017

View reviewed changes

adding the semester function that give the date interval of the semester

c667001

shex1627 force-pushed the semesterJobUtil branch from 630dcf4 to c667001 Compare May 26, 2017 00:23

shex1627 merged commit 568fca3 into ocf:master May 26, 2017

adding semester_dates function #58

adding semester_dates function #58

Conversation

shex1627 commented Mar 12, 2017

abizer left a comment

Choose a reason for hiding this comment

shex1627 commented Mar 12, 2017 • edited Loading

abizer commented Mar 12, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-mcallister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-mcallister commented Mar 20, 2017

shex1627 commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpengboy commented Apr 10, 2017

shex1627 commented Apr 10, 2017

shex1627 commented Apr 16, 2017

matthew-mcallister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskuehl Apr 16, 2017 • edited Loading

Choose a reason for hiding this comment

shex1627 commented Apr 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shex1627 Apr 30, 2017 • edited Loading

Choose a reason for hiding this comment

abizer commented Apr 30, 2017 via email

shex1627 commented Apr 30, 2017 • edited Loading

Choose a reason for hiding this comment

shex1627 commented May 21, 2017

matthew-mcallister left a comment

Choose a reason for hiding this comment

shex1627 commented May 24, 2017

matthew-mcallister commented May 24, 2017

shex1627 commented May 24, 2017

shex1627 commented May 26, 2017

matthew-mcallister commented May 26, 2017

shex1627 commented Mar 12, 2017 •

edited

Loading

chriskuehl Apr 16, 2017 •

edited

Loading

shex1627 Apr 30, 2017 •

edited

Loading

shex1627 commented Apr 30, 2017 •

edited

Loading