cesium-ml / cesium

Machine Learning Time-Series Platform
Other
670 stars 101 forks source link

Add note regarding MSVC #266

Closed filmor closed 6 years ago

filmor commented 6 years ago

This would have saved me quite a bit of time :)

codecov-io commented 6 years ago

Codecov Report

Merging #266 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #266   +/-   ##
=======================================
  Coverage   93.11%   93.11%           
=======================================
  Files          34       34           
  Lines        1903     1903           
  Branches      239      239           
=======================================
  Hits         1772     1772           
  Misses         97       97           
  Partials       34       34

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 11e9d66...f56e1e4. Read the comment docs.

stefanv commented 6 years ago

This should not be true; can you expand on the problem? We may be able to fix it quite easily.

filmor commented 6 years ago

Well, it is :)

The Lomb-Scargle code (e.g. https://github.com/cesium-ml/cesium/blob/master/cesium/features/_lomb_scargle.h#L81, but there are a lot of other places both in _lomb_scargle.h and _eigs.h) uses Variable Length Arrays which are explicitly not supported by MSVC (see https://stackoverflow.com/questions/5246900) as they are not valid C++.

If someone wanted to clean that up, the correct function to use instead would be https://msdn.microsoft.com/en-us/library/5471dc8s.aspx. Of course, if the size of those arrays could be limited in some way, you could just use staticly sized buffers instead.

stefanv commented 6 years ago

I believe you :)

I don't think there's any reason why we can't just allocate some memory here, instead of using the heap. Are you interested in helping out, or should I take a pass at it? This is the oldest part of the code base (came from a previous project), and hasn't seen much attention recently I'm afraid.

filmor commented 6 years ago

I currently don't have the time to work on this, a note in your README is all I can do right now ;)

If you are looking into this, consider dirching the implementation and instead using only scipy's or gatspy's. You depend on those anyway and this would make your package pure Python.

stefanv commented 6 years ago

We've thought about replacing this implementation, but it still offers some features not found in the others. But we'd certainly be open to that, eventually.