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

333 configure step count in dialogs #125

Merged
merged 29 commits into from
Jul 21, 2023

Conversation

bscheltinga
Copy link
Contributor

@bscheltinga
Copy link
Contributor Author

bscheltinga commented Jul 14, 2023

I spotted:

if pa_group is None:
pa_group = 2

I think it should be pa_group = 1

if previous_goal is None:
previous_goal = 15

Instead of setting it to a default of 15, I propose we use historical data to set a realistic goal.

@bscheltinga bscheltinga requested a review from wbaccinelli July 14, 2023 11:26
@bscheltinga
Copy link
Contributor Author

bscheltinga commented Jul 14, 2023

I realize that most functions I created will not work if no data is returned from the sensor data API. Also, I tested all functions in isolation, as spinning up the system would not work as there is no data.

Copy link
Contributor

@wbaccinelli wbaccinelli left a comment

Choose a reason for hiding this comment

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

Well done! I left some suggestions. Probably other things can be optimized, but this will work anyway

Comment on lines 215 to 217
group_2_threshold_total_steps = 56000
sufficient_daily_steps = 8000
group_2_threshold_daily_steps = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be better to move this values to the constants

user_id = int(tracker.current_state()['sender_id'])

end = datetime.datetime.now()
start = end - datetime.timedelta(days=16)
Copy link
Contributor

Choose a reason for hiding this comment

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

also the number of days for the different ranges would be better placed in the constants, so to have all defined in a single place

# Get data from API
user_id = int(tracker.current_state()['sender_id'])
end = datetime.datetime.now()
start = end - datetime.timedelta(days=16)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here


# Get avg. steps per day
end = datetime.now()
start = end - timedelta(days=9)
Copy link
Contributor

Choose a reason for hiding this comment

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

number of days could be a constant?

Rasa_Bot/actions/helper.py Outdated Show resolved Hide resolved
Comment on lines 1280 to 1281
min_val = 2000
max_val = 10000
Copy link
Contributor

Choose a reason for hiding this comment

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

Also this values should be moved to the constants

Comment on lines 1464 to 1467
step_goals = []
for i in range(7):
steps_nine_days = steps[i:i + 9]
steps_nine_days = [x for x in steps_nine_days if not pd.isna(x)] # Remove NaN values
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
step_goals = []
for i in range(7):
steps_nine_days = steps[i:i + 9]
steps_nine_days = [x for x in steps_nine_days if not pd.isna(x)] # Remove NaN values
steps_nine_days = [steps[i:i + 9] for i in range(7) if not pd.isna(steps[i:i + 9])] # Remove NaN values

Not tested, but I guess that this does the same thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this suggestion will not work. The for loop includes step_goals.append(int(round(steps_nine_days[5], -1))) # Definition of the goal

Rasa_Bot/actions/helper.py Outdated Show resolved Hide resolved
Copy link
Contributor

@wbaccinelli wbaccinelli left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

Comment on lines +5 to +6
import pandas as pd
import numpy as np
Copy link
Contributor

Choose a reason for hiding this comment

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

numpy and pandas should be added to the requirements file of sensor_api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch :)

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

4.0% 4.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@bscheltinga bscheltinga merged commit 7e127cd into main Jul 21, 2023
wbaccinelli added a commit that referenced this pull request Jul 24, 2023
…-step-count-in-dialogs"

This reverts commit 7e127cd, reversing
changes made to 0ee39e4.

Undoing sensors merge
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.

Configure step count with dialogues
2 participants