UCL / rsd-engineeringcourse

Course materials for Research Software Engineering course.
http://github-pages.ucl.ac.uk/rsd-engineeringcourse/
Other
108 stars 98 forks source link

Revisit design patterns #206

Closed stefpiatek closed 2 years ago

stefpiatek commented 2 years ago
ageorgou commented 2 years ago

Okay, the old provider of the sunspot data doesn't seem to offer it anymore. I've changed the function a bit to work with the original dataset. It's not quite the same data, though - this is monthly (~3000 entries), whereas the old one had ~300 entries. I've just seen that they have the yearly data too, though! @dpshelio would know more about what's more suitable...

Have pushed a commit for it (with the monthly data) and will push the yearly ones, feel free to revert either/both.

dpshelio commented 2 years ago

yuml works, don't know why it's not shown in the notebooks. This could be because they may provide now a SVG and Image from Ipython may not understand it. Ipython does have an SVG display tool. For the sunspots, we may not need to use the API, and use the link we have in the csv chapter - which I know it will last longer as is the official data provider. (which you had used in a function but then the class has the old one which the api doesn't respond anymore)

dpshelio commented 2 years ago

Oh! and :heart_eyes: sunpy is there! I feel it!!! :joy:

ageorgou commented 2 years ago

Changing IPython.display.Image to IPython.display.SVG seems to work for yuml!

stefpiatek commented 2 years ago

Great thanks both, I'll work on this either this afternoon or tomorrow.

Ah yes you're right, we're doing a simple factory and not a factory method specifically. We could have a factory method for reading images which you can then subclass for a factory to read astonomical image files and another child class as a factory method for microscopy images?

stefpiatek commented 2 years ago

Ended up going for the dataset as it was before from a file because was getting an error in the strategies. Also spent far too long to find out the lombscargle doesn't like a view of an array and needs it's own array. Will work on the final part there, but I think I've addressed the other changes.

Still to do:

stefpiatek commented 2 years ago

Just following up on this to see if we're happy with it. Maybe a bit too keen to get it in today if possible!

ageorgou commented 2 years ago

I'm fine with what I've looked at, which is as much as I'll be able to given the time frame :)