Skip to content

Modernize downsampling code#6

Open
tstenner wants to merge 2 commits into
masterfrom
modernfilter
Open

Modernize downsampling code#6
tstenner wants to merge 2 commits into
masterfrom
modernfilter

Conversation

@tstenner
Copy link
Copy Markdown
Collaborator

Move initialization code into the constructor so copy/move constructors can be generated automatically, move vector initialization into member initializers for fewer reallocations, add a std::map<int, Coeffs> for downsampling coefficients.

@tstenner tstenner requested a review from dmedine May 19, 2020 09:42
@tstenner tstenner mentioned this pull request May 19, 2020
@dmedine
Copy link
Copy Markdown
Collaborator

dmedine commented May 20, 2020

The refactoring looks quite fine to me, but I still need to test it.

Is there any reason to rebase the labstreaminglayer/App-BrainAmpSeries repo rather than move to the brainp-roducts/LSL-BrainAmpSeries repo altogether?

I ask because I am trying to make the Brain Products LSL solutions consistent and consolidated, finally, and rebasing back is contrary to my initial designs. The organization of these apps' repositories was done incorrectly from the beginning by putting each app for each Brain Products device in the same repository. This 'fault' stems from when the project migrated from google code to GitHub. The situation was made even worse (from my perspective) when the BrainAmpSeries app was put out on its own, but the rest were left together in the labstreaminglyaer/BrainProducts repository. (I'm not trying to assign blame or cast stones! I would have done the same thing in your shoes, but from my point of view it problematic.)

The other solution as I see it, is to break up the BrainProducts repo on the labstreaminglayer repository, rebase each app to the brain-products organization repositories, and then change the brain-products organization repositories to be submoduled to the labstreaminglayer master repository. Which, frankly is just a lot of effort for what seems to me to maintain a largely cosmetic difference.

Personally, I see this as clinging to the LSL app cosmoverse situation that we have now. I think the more vendors that want to peal off from the original repository the better. It makes things cleaner and easier to maintain.

@tstenner
Copy link
Copy Markdown
Collaborator Author

Is there any reason to rebase the labstreaminglayer/App-BrainAmpSeries repo rather than move to the brainp-roducts/LSL-BrainAmpSeries repo altogether?

For the lsl repo, it's just adding your changes in a single commit wheras in the brainproducts repo I'd have to either add a messy commit re-adding the readme changes and CMake / CI files or overwrite the recent changes (which I don't have any permissions for anyway).

Instead of forking the repository I suggest we transfer this repository to the BrainProducts organization so there's only the one official repository and hopefully github will forward old references to this repository to brainproducts/LSL-BrainAmpSeries

The organization of these apps' repositories was done incorrectly from the beginning by putting each app for each Brain Products device in the same repository.

Agreed.

The situation was made even worse (from my perspective) when the BrainAmpSeries app was put out on its own, but the rest were left together in the labstreaminglyaer/BrainProducts repository.

Also agreed, but it improved things for the BrainAmpSeries app considerably.

The other solution as I see it, is to break up the BrainProducts repo on the labstreaminglayer repository, rebase each app to the brain-products organization repositories, and then change the brain-products organization repositories to be submoduled to the labstreaminglayer master repository. Which, frankly is just a lot of effort for what seems to me to maintain a largely cosmetic difference.

I can check my notes for the git incantations to separate the subfolders into separate repositories. Those could then be hosted in the brainproducts organization. The real work is fixing all the links to the old App-BrainProducts repository to point to the new repositories.

In the beginning, it'd be mainly a cosmetic difference, but as soon as someone gets the idea to add a CI system it'll make everything a lot easier.

Personally, I see this as clinging to the LSL app cosmoverse situation that we have now. I think the more vendors that want to peal off from the original repository the better. It makes things cleaner and easier to maintain.

For me, it makes almost no difference where the repository is, as long as someone takes a look at my PRs and the CMake / CI config is there so I can get binaries for exotic platforms (e.g. Windows XP) and don't have to change anything to include the BrainAmp app in our internal lsl installer.

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.

2 participants