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

New Full Logbook #38

Merged
merged 16 commits into from
Jun 21, 2024
Merged

Conversation

PTschaikner
Copy link
Contributor

Summary

I have created a feature to extract a more exhaustive logbook for Aurora boards. You can now retrieve a CSV file that includes attempts on boulders that haven't been climbed yet.

New Logbook Fields

  • tries: Returns the tries in the session.
  • tries_total: Returns all tries on the same boulder (also taking angle and is_mirror into account).
  • sessions_count: Returns the number of sessions on the boulder.
  • logged_grade: Returns the grade that has been logged by the user.
  • displayed_grade: Returns the grade provided by the app (integrated because the difficulty is only logged when a climb has been completed successfully).
  • is_ascent: Indicates whether the boulder was climbed in this session.

These features represent the information I’d like to use for my own analysis, but I believe they might be useful for others as well.

Problems/Issues/Questions

  • displayed_grade Limitation: The displayed_grade only works when a local database is provided. I haven't found an elegant way to retrieve this information from the API without significant performance issues. One potential solution is to integrate results from get_climb_stats and use the most logged or median difficulty.
  • Testing Scope: The feature has only been tested with my own data from Grasshopper and Tension boards. More extensive testing might be needed to ensure robustness.
  • Backward Compatibility: There might be issues with how tries were saved in older versions where the mechanics for "tries" were different. My database doesn't include such sends, making it difficult to test scenarios where the lookup returns something like "3 months".
  • Code Quality: The code is not very elegant at this point and could benefit from further refactoring and cleanup.

Recommendations

To balance functionality and performance, I propose maintaining two separate functions:

  • Basic Logbook: For general use without a requirement for a local database.

  • Detailed Logbook: For more in-depth analysis, requiring a local database to avoid performance issues.

Currently, the full logbook returns NA for displayed_grade when no local database is available, resulting in slow performance. Using a local database improves speed significantly.

Conclusion

If these features align with your goals for the project, I am happy to refine the code based on your feedback and suggestions. Thank you for the opportunity to contribute. I look forward to building visualizations with this data.

Copy link
Owner

@lemeryfertitta lemeryfertitta left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

To address some of your concerns/questions:

The displayed_grade only works when a local database is provided

That seems fine to me for now, let's just make a note of it in the command help and README.

Testing Scope: The feature has only been tested with my own data from Grasshopper and Tension boards. More extensive testing might be needed to ensure robustness.

I will test it out at some point and make sure it plays nice with the old ascents

Code Quality: The code is not very elegant at this point and could benefit from further refactoring and cleanup.

Made a few suggestions, we can iterate on this a bit, I'm happy to help.

I propose maintaining two separate functions:

I think that I prefer one function that does it all, but with the optional database as you've provided already.

I look forward to building visualizations with this data.

If you end up building any cool visualizations that seem like they may fit within the library, feel free to push those here! I think others may find them useful.


def main():
parser = argparse.ArgumentParser()
subparsers = parser.add_subparsers(dest="command", required=True)
add_logbook_parser(subparsers)
add_full_logbook_parser(subparsers) # Add this line
Copy link
Owner

@lemeryfertitta lemeryfertitta Jun 12, 2024

Choose a reason for hiding this comment

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

Rather than maintaining two logbook commands, let's just get your features into the original command.

For Moonboard logbooks, we can just return null in the new fields for now, with the idea of eventually implementing them or just leaving them null if they don't apply. I think it's fine to leave fields null if they require a database. We can update the README and/or command help to explain the differences.

src/boardlib/api/aurora.py Outdated Show resolved Hide resolved
src/boardlib/api/aurora.py Outdated Show resolved Hide resolved
src/boardlib/api/aurora.py Outdated Show resolved Hide resolved
src/boardlib/api/aurora.py Outdated Show resolved Hide resolved
src/boardlib/api/aurora.py Outdated Show resolved Hide resolved
src/boardlib/api/aurora.py Outdated Show resolved Hide resolved
src/boardlib/api/aurora.py Outdated Show resolved Hide resolved
@lemeryfertitta
Copy link
Owner

Tested locally - I actually have two accounts, one from pre-bids and one from post-bids. I'm not sure if this is something that all pre-bids users had to do or if it was just me forgetting my account.

Anyways, post-bids behavior is working well. Pre-bids fails with the following error:

File ".../boardlib/api/aurora.py", line 399, in get_bids_logbook
    return sync_results["PUT"]["bids"]
KeyError: 'bids'

So maybe we can just try/except on the attempt to get bids to fall back on the current behavior in that case. Once that error is fixed and #38 (comment) is addressed, I think this PR will be good to go and we can get these features released.

@PTschaikner
Copy link
Contributor Author

Thanks for testing! I was able to recreate your error by providing a database with an empty bids table. There's a similar issue with an empty ascents table, though it's unlikely to occur. In both cases, I think returning an empty table would be better than raising an unspecific error. If you have the time, it would be great if you could test whether this fix works with your old account.

Before merging the logbook commands, it would be helpful if you could test the basic functionality of the moon logbook. I created an account with a dummy logbook using the current moonboard app, but I couldn't get it to work with your original command, even after rolling back to an older commit.

Are the tests up to date? I encountered several errors when running them. I'd like to experiment with writing my first tests, and this project seems like a good starting point, if you don't mind.

@lemeryfertitta
Copy link
Owner

In both cases, I think returning an empty table would be better than raising an unspecific error. If you have the time, it would be great if you could test whether this fix works with your old account.

Good idea, better than my suggestion. I tested your fix and it works perfectly for both of my accounts.

Before merging the logbook commands, it would be helpful if you could test the basic functionality of the moon logbook. I created an account with a dummy logbook using the current moonboard app, but I couldn't get it to work with your original command, even after rolling back to an older commit.

I tested and the moon logbook is indeed broken. A quick investigation suggests that moon has recently changed their site. The API looks to be mostly the same but I think they changed the login flow. I'll create a separate ticket for this. Since that is broken anyways, let's still merge your functionality into the main logbook command. Don't worry about breaking Moon more since it is already broken.

Are the tests up to date? I encountered several errors when running them. I'd like to experiment with writing my first tests, and this project seems like a good starting point, if you don't mind.

Please feel free to improve/add to the tests. I used them when I worked on the first release of this library but as changes were made they were ignored. If you are motivated, it would be great to get them fixed up and eventually add them to Github CI and potentially block merging for changes with failing tests.

@PTschaikner
Copy link
Contributor Author

I have made the following updates in this Pull Request:

  • Unified Logbook Command: Integrated the new version for a more streamlined experience.
  • Bug Fixes: Resolved minor bugs, including issues with grade conversion.
  • Updated Unit Tests: Ensured the unit tests are up to date and functional.
  • Documentation: Added detailed information about the new logbook features in the README.md.

Future Improvements I Plan to Work On:

  • Enhanced Error Handling: Focusing on login errors and issues with incorrect database files.
  • Expanded Unit Tests: Planning to add more comprehensive tests to improve reliability.
  • Visualizations: Developing D3.js visualizations for logbook data. I aim to create a solution that allows users to view a static .html file without needing to run a local server.

Updated the retrieval of grade_info to handle None values for the difficulty.
This ensures that if difficulty is None, the corresponding value from the grades_dict is retrieved using None as the key.
Copy link
Owner

@lemeryfertitta lemeryfertitta left a comment

Choose a reason for hiding this comment

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

Great, looking good to me! I'm happy to get this one merged and released, or we can keep it open for any of the future improvements you had in mind. Let me know what you prefer.

@PTschaikner
Copy link
Contributor Author

Feel free to merge, especially if it is useful for Climbdex as suggested by @gardaholm. I hope it doesn't cause any headaches there. I'll focus on the visualizations and open a separate PR if I manage to get to a point that seems useful in this regard.

@PTschaikner PTschaikner marked this pull request as ready for review June 20, 2024 21:54
@lemeryfertitta lemeryfertitta merged commit 802fb23 into lemeryfertitta:main Jun 21, 2024
Comment on lines +544 to +547
def logbook_entries(board, username, password, grade_type="font", db_path=None):
login_info = login(board, username, password)
token = login_info["token"]
user_id = login_info["user_id"]
Copy link
Contributor

Choose a reason for hiding this comment

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

can we go straight for the token here instead of username, password? For climbdex we only save the bearer token after the initial login


final_logbook = combine_ascents_and_bids(ascents_df, bids_summary, db_path, grades_dict, grade_type)

full_logbook_df = pd.DataFrame(final_logbook, columns=['board', 'angle', 'climb_name', 'date', 'logged_grade', 'displayed_grade', 'difficulty', 'tries', 'is_mirror', 'is_ascent', 'comment'])
Copy link
Contributor

Choose a reason for hiding this comment

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

adding the climb_uuid would be pretty helpful here
This would simplify the attempts lookup for climbdex a lot: lemeryfertitta/Climbdex#55

full_logbook_df = pd.DataFrame(final_logbook, columns=['climb_uuid', 'board', 'angle', 'climb_name', 'date', 'logged_grade', 'displayed_grade', 'difficulty', 'tries', 'is_mirror', 'is_ascent', 'comment'])

Copy link
Owner

Choose a reason for hiding this comment

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

I think that is a fine suggestion to make this more useful for Climbdex, but I would want to make sure we don't do that in a way that clutters the output of the boardlib logbook command. Realistically, users of the command line version don't need the climb UUID in the output CSV.

One idea would be to refactor out a version of this function that does everything you've suggested here, and then in the command-line version we can handle the extra steps. From my understanding, that would just be to login and get a token and also to filter out the climb_uuid. I'd be happy to accept a PR for this if it would help with your proposed climbdex additions!

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, will do it this way!

full_logbook_df = pd.DataFrame(final_logbook, columns=['board', 'angle', 'climb_name', 'date', 'logged_grade', 'displayed_grade', 'difficulty', 'tries', 'is_mirror', 'is_ascent', 'comment'])
full_logbook_df['date'] = pd.to_datetime(full_logbook_df['date'])

full_logbook_df = full_logbook_df.groupby(['climb_name', 'is_mirror', 'angle']).apply(calculate_sessions_count).reset_index(drop=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

full_logbook_df = full_logbook_df.groupby(['climb_uuid', 'climb_name', 'is_mirror', 'angle']).apply(calculate_sessions_count).reset_index(drop=True)

full_logbook_df['date'] = pd.to_datetime(full_logbook_df['date'])

full_logbook_df = full_logbook_df.groupby(['climb_name', 'is_mirror', 'angle']).apply(calculate_sessions_count).reset_index(drop=True)
full_logbook_df = full_logbook_df.groupby(['climb_name', 'is_mirror', 'angle']).apply(calculate_tries_total).reset_index(drop=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

full_logbook_df = full_logbook_df.groupby(['climb_uuid', 'climb_name', 'is_mirror', 'angle']).apply(calculate_tries_total).reset_index(drop=True)

full_logbook_df = full_logbook_df.groupby(['climb_name', 'is_mirror', 'angle']).apply(calculate_sessions_count).reset_index(drop=True)
full_logbook_df = full_logbook_df.groupby(['climb_name', 'is_mirror', 'angle']).apply(calculate_tries_total).reset_index(drop=True)

full_logbook_df['is_repeat'] = full_logbook_df.duplicated(subset=['climb_name', 'is_mirror', 'angle'], keep='first')
Copy link
Contributor

Choose a reason for hiding this comment

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

full_logbook_df['is_repeat'] = full_logbook_df.duplicated(subset=['climb_uuid', 'climb_name', 'is_mirror', 'angle'], keep='first')

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.

3 participants