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

Export survey module data #23

Closed
wants to merge 37 commits into
base: development
from

Conversation

Projects
None yet
2 participants
@CdavM
Contributor

CdavM commented Jul 6, 2016

This is a command that exports a graph of the survey. It currently only exports data from the latest datapoint. This PR should be used to discuss implementation details and additional features for this script.

@CdavM

This comment has been minimized.

Show comment
Hide comment
@CdavM

CdavM Jul 6, 2016

Contributor

Right now we're simply adding all vertices and edges into one big graph. We could also create a networkX graph structure and export that. Would this be better?

Contributor

CdavM commented Jul 6, 2016

Right now we're simply adding all vertices and edges into one big graph. We could also create a networkX graph structure and export that. Would this be better?

@CdavM CdavM closed this Jul 8, 2016

@CdavM CdavM reopened this Jul 8, 2016

exported_graph = {
'graph': meta_graph,
'friendly_nodes': friendly_nodes,
'timepoint': str(latest_data_timepoint)

This comment has been minimized.

@mitar

mitar Jul 14, 2016

Member

timestamp?

@mitar

mitar Jul 14, 2016

Member

timestamp?

This comment has been minimized.

@mitar

mitar Jul 14, 2016

Member

Do you have to convert it to string? JSON can serialize datetime, or not?

@mitar

mitar Jul 14, 2016

Member

Do you have to convert it to string? JSON can serialize datetime, or not?

This comment has been minimized.

@CdavM

CdavM Jul 18, 2016

Contributor

TypeError: datetime.datetime(2016, 7, 18, 5, 18, 51, tzinfo=<bson.tz_util.FixedOffset object at 0x7f0519ff0f50>) is not JSON serializable
:-(

@CdavM

CdavM Jul 18, 2016

Contributor

TypeError: datetime.datetime(2016, 7, 18, 5, 18, 51, tzinfo=<bson.tz_util.FixedOffset object at 0x7f0519ff0f50>) is not JSON serializable
:-(

This comment has been minimized.

@mitar

mitar Jul 22, 2016

Member

This is probably why you should use Django JSON serializer: https://github.com/django/django/blob/master/django/core/serializers/json.py

@mitar

mitar Jul 22, 2016

Member

This is probably why you should use Django JSON serializer: https://github.com/django/django/blob/master/django/core/serializers/json.py

@CdavM

This comment has been minimized.

Show comment
Hide comment
@CdavM

CdavM Jul 18, 2016

Contributor

I made almost all of the changes requested. I am still going to ask users to enter the date in a human-readable format. Otherwise they have to enter the date somewhere to get the timestamp out, which is really inconvenient. I hid the implementation details about the datetime module from users. I am still using UTC. I am using the datetime module from python instead of django_utils because datetime is more common + used elsewhere in nodewatcher.

Contributor

CdavM commented Jul 18, 2016

I made almost all of the changes requested. I am still going to ask users to enter the date in a human-readable format. Otherwise they have to enter the date somewhere to get the timestamp out, which is really inconvenient. I hid the implementation details about the datetime module from users. I am still using UTC. I am using the datetime module from python instead of django_utils because datetime is more common + used elsewhere in nodewatcher.

@CdavM

This comment has been minimized.

Show comment
Hide comment
@CdavM

CdavM Jul 18, 2016

Contributor

OK I now added an option to feed the data to the rogue node detection algorithm. But this requires an import statement to the rogue node algorithm. Which I can't commit because the rogue node algorithm hasn't been merged yet. So I think we should first merge this, then work on merging the rogue node detection and then I can push a final commit here.

Contributor

CdavM commented Jul 18, 2016

OK I now added an option to feed the data to the rogue node detection algorithm. But this requires an import statement to the rogue node algorithm. Which I can't commit because the rogue node algorithm hasn't been merged yet. So I think we should first merge this, then work on merging the rogue node detection and then I can push a final commit here.

@CdavM

This comment has been minimized.

Show comment
Hide comment
@CdavM

CdavM Jul 19, 2016

Contributor

Can I get another code review?

Contributor

CdavM commented Jul 19, 2016

Can I get another code review?

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Jul 22, 2016

Member

I am still going to ask users to enter the date in a human-readable format.

Yes, that was the whole point. You ask them in human-readable format (where allowing them to enter timestamp in local timezone is additional thing which makes them even more user-friendly, with optional timezone), but then you quickly convert this to datetime and you use that all around Python.

I am using the datetime module from python instead of django_utils because datetime is more common + used elsewhere in nodewatcher.

???

Those things are complementary. Nodewatcher is written in Django, so using Django-provided functions help make it better. For example, it allows one to use settings to configure some aspects of data parsing and formatting and you can then through settings decide if you want or do not want to use timezones.

Member

mitar commented Jul 22, 2016

I am still going to ask users to enter the date in a human-readable format.

Yes, that was the whole point. You ask them in human-readable format (where allowing them to enter timestamp in local timezone is additional thing which makes them even more user-friendly, with optional timezone), but then you quickly convert this to datetime and you use that all around Python.

I am using the datetime module from python instead of django_utils because datetime is more common + used elsewhere in nodewatcher.

???

Those things are complementary. Nodewatcher is written in Django, so using Django-provided functions help make it better. For example, it allows one to use settings to configure some aspects of data parsing and formatting and you can then through settings decide if you want or do not want to use timezones.

@mitar mitar referenced this pull request Jul 22, 2016

Merged

Export survey module data #26

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Jul 22, 2016

Member

I made a new pull request from a branch in this repository so that I can simply fix some things instead of going through long comments iterations.

I think you made great stuff, but I just changed some wording to make it clearer (in my personal opinion).

Please check that pull request and if you are OK with it I will merge it.

Member

mitar commented Jul 22, 2016

I made a new pull request from a branch in this repository so that I can simply fix some things instead of going through long comments iterations.

I think you made great stuff, but I just changed some wording to make it clearer (in my personal opinion).

Please check that pull request and if you are OK with it I will merge it.

@mitar mitar closed this Jul 22, 2016

@mitar

This comment has been minimized.

Show comment
Hide comment
@mitar

mitar Jul 22, 2016

Member

I have also made you a contributor to this repository. So from now on create branches inside this repository. (But you cannot push to development branch directly. And you should not. Still create pull requests to the development branch.)

Member

mitar commented Jul 22, 2016

I have also made you a contributor to this repository. So from now on create branches inside this repository. (But you cannot push to development branch directly. And you should not. Still create pull requests to the development branch.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment