-
Notifications
You must be signed in to change notification settings - Fork 15
Add New Features and Agents to Ad Server #8
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi @zoeshao0425, thanks for the PR - that's really great! |
Hi @falox, No worries at all, take your time! I'm glad that I had the opportunity to contribute to this amazing ad-server. Writing the code and working on the pull request has been a great learning experience for me. The concise design and beautiful visualizations of the ad-server are definitely something I admire. I appreciate the chance to be involved, and I'm looking forward to any feedback you might have on my PR when you have time. Thanks again! |
|
Hi @zoeshao0425, I'm resuming this review. Can you display the revenue chart in the same CTR chart, using the seconday axis? This would better display the correlation between CTR and revenue over time |
| time_of_day = impressions / self.max_impressions | ||
| return time_of_day | ||
|
|
||
| def get_ad_type_click_probability(self, ad_type, time_of_day): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this can be simpler (I mean more readable). Anyway, can you please comment this method explaining the logic?
| # Your custom logic to generate budget based on ad_type | ||
| return self.ads_random.uniform(500, 1000) | ||
|
|
||
| def get_current_time_of_day(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename to get_current_time. It's a generic time T, not necessarily a cyclic 24h time
Co-authored-by: Alberto Falossi <26248823+falox@users.noreply.github.com>
Hi @falox, thank you for your feedback!! I really appreciate it! This is my senior year in college, and I am way busier than I expected, so I have not got enough time to go through these comments. Sorry about that! I will look into them more thoroughly and implemented the changes you suggested during winter break if you don't mind :) |
@zoeshao0425 Absolutely, take your time Zoe. As you can see, this repo is maintained on a best effort basis |
Hi there, my name is Zoe. This pull request introduces several new features and agents to the ad server, enhancing its capabilities and better simulating a real-world online advertising environment. The following changes have been made:
These changes will provide a more comprehensive and realistic simulation of online advertising, allowing for better evaluation of various agents' performance in different scenarios. One limitation I need to acknowledge is that there are a few constants that could be changed to better simulate real scenario. Maybe these could be added into parser, allowing the users to modify more easily.
Please consider merging this pull request to enhance the capabilities of the ad server. If you have any questions or suggestions, feel free to comment.
Thank you for your consideration!